From fed4275403345d2bdaf6bda91e5a74bd0c1e2902 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Nov 2017 12:15:26 +0000 Subject: [PATCH 1/3] Factor out code that gets message content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The nesting is getting pretty deep here. Let’s make it into its own method so it doesn’t get out of hand when we add more functionality to it. --- app/main/views/conversation.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/app/main/views/conversation.py b/app/main/views/conversation.py index abdd416de..3d1c7ccc4 100644 --- a/app/main/views/conversation.py +++ b/app/main/views/conversation.py @@ -120,12 +120,7 @@ def get_sms_thread(service_id, user_number): yield { 'inbound': is_inbound, 'content': SMSPreviewTemplate( - { - 'content': ( - notification['content'] if is_inbound else - notification['template']['content'] - ) - }, + {'content': get_sms_content(notification, is_inbound)}, notification.get('personalisation'), downgrade_non_gsm_characters=(not is_inbound), redact_missing_personalisation=redact_personalisation, @@ -134,3 +129,10 @@ def get_sms_thread(service_id, user_number): 'status': notification.get('status'), 'id': notification['id'], } + + +def get_sms_content(notification, is_inbound): + return ( + notification['content'] if is_inbound else + notification['template']['content'] + ) From f6950ae987b9cd829ee6ccf552b8ab60699b8820 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Nov 2017 12:36:26 +0000 Subject: [PATCH 2/3] Stop escaping special characters in inbound MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At least one of our providers gives us messages with special characters escaped, ie a newline comes through as `\n`, not a literal newline. We shouldn’t be showing these backslashes to any of our users. Python has built in codecs for dealing with encoding/decoding of strings – see https://docs.python.org/3/library/codecs.html#text-encodings for details. Using these builtins is safer than trying to do anything regex or parsing-based. --- app/main/views/conversation.py | 2 +- app/main/views/dashboard.py | 3 +++ tests/app/main/views/test_conversation.py | 23 +++++++++++++++++++++++ tests/app/main/views/test_dashboard.py | 19 +++++++++++++++++++ tests/conftest.py | 20 ++++++++++++++++++++ 5 files changed, 66 insertions(+), 1 deletion(-) diff --git a/app/main/views/conversation.py b/app/main/views/conversation.py index 3d1c7ccc4..e975fe244 100644 --- a/app/main/views/conversation.py +++ b/app/main/views/conversation.py @@ -133,6 +133,6 @@ def get_sms_thread(service_id, user_number): def get_sms_content(notification, is_inbound): return ( - notification['content'] if is_inbound else + bytes(notification['content'], "utf-8").decode('unicode_escape') if is_inbound else notification['template']['content'] ) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index ce63fdc00..b81da435f 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -203,6 +203,9 @@ def get_inbox_partials(service_id): format_phone_number_human_readable(message['user_number']) for message in messages_to_show }: + message.update({ + 'content': bytes(message['content'], 'utf-8').decode('unicode_escape') + }) messages_to_show.append(message) if not inbound_messages: diff --git a/tests/app/main/views/test_conversation.py b/tests/app/main/views/test_conversation.py index dba58a14c..f4edb3685 100644 --- a/tests/app/main/views/test_conversation.py +++ b/tests/app/main/views/test_conversation.py @@ -161,6 +161,29 @@ def test_view_conversation( ) == expected +def test_escaped_characters_in_inbound_messages( + client_request, + mock_get_notification, + mock_get_notifications, + mock_get_inbound_sms_with_special_characters, + fake_uuid, +): + + page = client_request.get( + 'main.conversation', + service_id=SERVICE_ONE_ID, + notification_id=fake_uuid, + ) + + assert normalize_spaces( + str(page.select_one('.sms-message-inbound .sms-message-wrapper')) + ) == ( + "
" + "the first line's content
the second line's content " + "
" + ) + + def test_view_conversation_updates( logged_in_client, mocker, diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 35a68dea4..b1787c691 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -172,6 +172,25 @@ def test_inbox_showing_inbound_messages( ) +def test_inbox_handles_escaped_characters( + client_request, + service_one, + mock_get_inbound_sms_with_special_characters, +): + + service_one['permissions'] = ['inbound_sms'] + + page = client_request.get('main.inbox', service_id=SERVICE_ONE_ID) + + assert normalize_spaces( + str(page.select_one('tbody tr .file-list-hint')) + ) == ( + "" + "the first line's content the second line's content" + "" + ) + + def test_empty_inbox( logged_in_client, service_one, diff --git a/tests/conftest.py b/tests/conftest.py index e1341994e..b61f228a2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1762,6 +1762,26 @@ def mock_get_inbound_sms(mocker): ) +@pytest.fixture(scope='function') +def mock_get_inbound_sms_with_special_characters(mocker): + def _get_inbound_sms( + service_id, + user_number=None, + ): + return [{ + 'user_number': '07900900001', + 'notify_number': '07900000002', + 'content': "the first line\\'s content\\nthe second line\\'s content", + 'created_at': datetime.utcnow().isoformat(), + 'id': sample_uuid(), + }] + + return mocker.patch( + 'app.service_api_client.get_inbound_sms', + side_effect=_get_inbound_sms, + ) + + @pytest.fixture(scope='function') def mock_get_inbound_sms_with_no_messages(mocker): def _get_inbound_sms( From f329e138cd50e18d904ef23d5829fe9d6e77248f Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Nov 2017 13:22:46 +0000 Subject: [PATCH 3/3] Factor out string escaping code So that it only lives in one place. --- app/main/views/conversation.py | 4 ++-- app/main/views/dashboard.py | 3 ++- app/utils.py | 4 ++++ 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/app/main/views/conversation.py b/app/main/views/conversation.py index e975fe244..53c79372a 100644 --- a/app/main/views/conversation.py +++ b/app/main/views/conversation.py @@ -10,7 +10,7 @@ from notifications_utils.recipients import format_phone_number_human_readable from notifications_utils.template import SMSPreviewTemplate from app.main import main from app.main.forms import SearchTemplatesForm -from app.utils import user_has_permissions +from app.utils import user_has_permissions, unescape_string from app import notification_api_client, service_api_client from notifications_python_client.errors import HTTPError @@ -133,6 +133,6 @@ def get_sms_thread(service_id, user_number): def get_sms_content(notification, is_inbound): return ( - bytes(notification['content'], "utf-8").decode('unicode_escape') if is_inbound else + unescape_string(notification['content']) if is_inbound else notification['template']['content'] ) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index b81da435f..5b59ff912 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -31,6 +31,7 @@ from app.utils import ( FAILURE_STATUSES, REQUESTED_STATUSES, Spreadsheet, + unescape_string, ) @@ -204,7 +205,7 @@ def get_inbox_partials(service_id): for message in messages_to_show }: message.update({ - 'content': bytes(message['content'], 'utf-8').decode('unicode_escape') + 'content': unescape_string(message['content']) }) messages_to_show.append(message) diff --git a/app/utils.py b/app/utils.py index 3a69f4486..e446f9b94 100644 --- a/app/utils.py +++ b/app/utils.py @@ -379,3 +379,7 @@ def get_cdn_domain(): domain = parsed_uri.netloc[len(subdomain + '.'):] return "static-logos.{}".format(domain) + + +def unescape_string(string): + return bytes(string, "utf-8").decode('unicode_escape')