From 66e6538d95ecee90ff6e6302b71a3ea7c548fce7 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 30 Apr 2018 10:06:33 +0100 Subject: [PATCH 1/4] Store original filename as S3 metadata MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit By doing this we no longer have to store it in the session. This is the last thing that’s currently in the session, so removing it means we can drop session storage for file uploads entirely. --- app/main/views/send.py | 1 + tests/app/main/views/test_send.py | 3 +++ 2 files changed, 4 insertions(+) diff --git a/app/main/views/send.py b/app/main/views/send.py index dd9d22231..1e01370e0 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -555,6 +555,7 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_ notification_count=len(recipients), template_id=str(template_id), valid=True, + original_file_name=request.args.get('original_file_name'), ) else: session['file_uploads'].pop(upload_id) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 15b2fb5e5..41e172ab2 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -590,6 +590,7 @@ def test_upload_valid_csv_shows_preview_and_table( service_id=SERVICE_ONE_ID, template_id=fake_uuid, upload_id=fake_uuid, + original_file_name='example.csv', **extra_args ) @@ -599,6 +600,7 @@ def test_upload_valid_csv_shows_preview_and_table( notification_count=3, template_id=fake_uuid, valid=True, + original_file_name='example.csv', ) assert page.h1.text.strip() == 'Preview of Two week reminder' @@ -1506,6 +1508,7 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( notification_count=53, template_id=fake_uuid, valid=True, + original_file_name='valid.csv', ) content = response.get_data(as_text=True) From 735d5f0a295eaf3844e5b1334a7116a82293d01b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 30 Apr 2018 10:04:06 +0100 Subject: [PATCH 2/4] Remove redundant call to get_template MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’re getting the template just to get back its `id`, which is the one thing we do know in order to get it. The call to get template is still happening inside `_check_messages`, so we’ll still catch someone trying to look at this page for a template that doesn’t exist. --- app/main/views/send.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 1e01370e0..76faba72f 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -589,9 +589,8 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_ @login_required @user_has_permissions('send_messages', restrict_admin_usage=True) def check_messages(service_id, template_id, upload_id, row_index=2): - db_template = service_api_client.get_service_template(service_id, template_id)['data'] - data = _check_messages(service_id, db_template['id'], upload_id, row_index) + data = _check_messages(service_id, template_id, upload_id, row_index) if ( data['recipients'].too_many_rows or From bc8bc727f38c24f45ee14eb0ff4101d2944abca3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 30 Apr 2018 10:46:39 +0100 Subject: [PATCH 3/4] Limit length of filename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit S3 has a limit of 2kb for metadata: > the user-defined metadata is limited to 2 KB in size. The size of > user-defined metadata is measured by taking the sum of the number of > bytes in the UTF-8 encoding of each key and value. – https://docs.aws.amazon.com/AmazonS3/latest/dev/UsingMetadata.html#object-metadata This means we have a limit of 1870 bytes for the filename: ```python encoded = 'notification_count50000template_id665d26e7-ceac-4cc5-82ed-63d773d21561validTrueoriginal_file_name'.encode('utf-8') sys.getsizeof(b) >>> 130 2000-130 >>> 1870 ``` Or, in other words, ~918 characters: ```python sys.getsizeof(('ü'*918).encode('utf-8')) >>> 1869 ``` --- app/main/views/send.py | 6 ++++- app/utils.py | 5 ++++ tests/app/main/views/test_send.py | 41 +++++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 1 deletion(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 76faba72f..954a3a1cb 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -46,6 +46,7 @@ from app.utils import ( get_errors_for_csv, get_help_argument, get_template, + unicode_truncate, user_has_permissions, ) @@ -555,7 +556,10 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_ notification_count=len(recipients), template_id=str(template_id), valid=True, - original_file_name=request.args.get('original_file_name'), + original_file_name=unicode_truncate( + request.args.get('original_file_name', ''), + 1872 + ), ) else: session['file_uploads'].pop(upload_id) diff --git a/app/utils.py b/app/utils.py index 3aeb6f373..ddf023f86 100644 --- a/app/utils.py +++ b/app/utils.py @@ -568,3 +568,8 @@ class GovernmentEmailDomain(AgreementInfo): )) except StopIteration: raise NotGovernmentEmailDomain() + + +def unicode_truncate(s, length): + encoded = s.encode('utf-8')[:length] + return encoded.decode('utf-8', 'ignore') diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 41e172ab2..21abc0958 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -1,4 +1,5 @@ # -*- coding: utf-8 -*- +import sys import uuid from functools import partial from glob import glob @@ -662,6 +663,46 @@ def test_upload_valid_csv_shows_preview_and_table( assert normalize_spaces(str(row.select('td')[index + 1])) == cell +def test_file_name_truncated_to_fit_in_s3_metadata( + client_request, + mocker, + mock_get_live_service, + mock_get_service_template_with_placeholders, + mock_get_users_by_service, + mock_get_detailed_service_for_today, + mock_s3_set_metadata, + fake_uuid, +): + + with client_request.session_transaction() as session: + session['file_uploads'] = { + fake_uuid: {'template_id': fake_uuid} + } + + mocker.patch('app.main.views.send.s3download', return_value=""" + phone number,name,thing,thing,thing + 07700900001, A, foo, foo, foo + """) + + file_name = 'ü😁' * 2000 + + client_request.get( + 'main.check_messages', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + upload_id=fake_uuid, + original_file_name=file_name, + ) + assert sys.getsizeof( + file_name.encode('utf-8') + ) > 2000 + + assert sys.getsizeof(''.join(( + '{}{}'.format(key, value) for key, value in + mock_s3_set_metadata.call_args_list[0][1].items() + )).encode('utf-8')) == 1998 + + def test_show_all_columns_if_there_are_duplicate_recipient_columns( client_request, mocker, From ef4dd9d12680368d2abf4354e59e8a9703d9c0a1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 30 Apr 2018 11:31:50 +0100 Subject: [PATCH 4/4] Add some breathing room to file name length limit Because Amazon stores metadata keys prefixed with `x-amz-` which might get counted as part of the size. --- app/main/views/send.py | 2 +- tests/app/main/views/test_send.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 954a3a1cb..21f09d657 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -558,7 +558,7 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_ valid=True, original_file_name=unicode_truncate( request.args.get('original_file_name', ''), - 1872 + 1600, ), ) else: diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 21abc0958..b9fb71350 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -700,7 +700,7 @@ def test_file_name_truncated_to_fit_in_s3_metadata( assert sys.getsizeof(''.join(( '{}{}'.format(key, value) for key, value in mock_s3_set_metadata.call_args_list[0][1].items() - )).encode('utf-8')) == 1998 + )).encode('utf-8')) == 1724 def test_show_all_columns_if_there_are_duplicate_recipient_columns(