From 0ce7f72b07030c61e1c0354e9afe9dd528cf6f08 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 7 Dec 2021 12:39:37 +0000 Subject: [PATCH] Reject CSV / Spreadsheet files larger than 10Mb This is a quick additional check to protect the user: - From getting a CloudFront 502 error if the file takes too long to upload. I was surprised to find it takes about 1 minute to upload a 70Mb file to S3.* - From getting a CloudFront 502 error when we follow the redirect and run through the slow processing code in utils that builds a RecipientCSV [1]. For context, a CSV with 100K rows and a few columns is around 5Mb, so a 10Mb limit should be enough. Analysis over the past week shows that the vast majority of CSV uploads are actually < 2.5Mb. I haven't added any tests for this because: - The check isn't critical, as the worst case scenario is the user gets a worse error than this in-app one. - There's no easy way to mock the validation, and I didn't want to have a test that depends on a 10Mb+ file. *We're using "key.put" to upload the file, when we could be doing a multipart upload [2]. However, I tried this myself with a chunk size of 1000 bytes and found it only led to a marginal improvement. [1]: https://github.com/alphagov/notifications-utils/pull/930 [2]: https://boto3.amazonaws.com/v1/documentation/api/latest/guide/s3-uploading-files.html --- app/main/forms.py | 11 +++++++++-- tests/app/main/views/test_send.py | 20 ++++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 11f77be1b..344bee3b6 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -9,6 +9,7 @@ from flask_login import current_user from flask_wtf import FlaskForm as Form from flask_wtf.file import FileAllowed from flask_wtf.file import FileField as FileField_wtf +from flask_wtf.file import FileSize from notifications_utils.columns import Columns from notifications_utils.countries.data import Postage from notifications_utils.formatters import strip_all_whitespace @@ -1512,8 +1513,14 @@ class ChangePasswordForm(StripWhitespaceForm): class CsvUploadForm(StripWhitespaceForm): - file = FileField('Add recipients', validators=[DataRequired( - message='Please pick a file'), CsvFileValidator()]) + file = FileField('Add recipients', validators=[ + DataRequired(message='Please pick a file'), + CsvFileValidator(), + FileSize( + max_size=10e6, # 10Mb + message='File must be smaller than 10Mb' + ) + ]) class ChangeNameForm(StripWhitespaceForm): diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 584cbc317..a919274c0 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -5,6 +5,7 @@ from glob import glob from io import BytesIO from itertools import repeat from os import path +from random import randbytes from unittest.mock import ANY from uuid import uuid4 from zipfile import BadZipFile @@ -966,6 +967,25 @@ def test_upload_csv_invalid_extension( assert "invalid.txt is not a spreadsheet that Notify can read" in resp.get_data(as_text=True) +def test_upload_csv_size_too_big( + logged_in_client, + mock_login, + service_one, + mock_get_service_template, + fake_uuid, +): + + resp = logged_in_client.post( + url_for('main.send_messages', service_id=service_one['id'], template_id=fake_uuid), + data={'file': (BytesIO(randbytes(11_000_000)), 'invalid.csv')}, + content_type='multipart/form-data', + follow_redirects=True + ) + + assert resp.status_code == 200 + assert "File must be smaller than 10Mb" in resp.get_data(as_text=True) + + def test_upload_valid_csv_redirects_to_check_page( client_request, mock_get_service_template_with_placeholders,