From 70badae5759a67527f68c7dcc434b3eb0ecc87ee Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 11 Dec 2017 16:02:12 +0000 Subject: [PATCH 1/3] Strip whitespace before validating any form MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’ve had whitespace-in-emails problems on: - register form - sign in form - send one off form We should just handle whitespace gracefully, by ignoring it. Makes sense to do this at a global level because there might be other places it’s causing problems that we don’t know about. The only place that users _might_ be relying on leading/trailing spaces (and we have no way of knowing this) is in passwords or tokens. So this filter ignores any instances of `PasswordField`. Adapted from @pcraig3’s work on Digital Marketplace: - https://github.com/alphagov/digitalmarketplace-supplier-frontend/commit/4b06f5cfb703678f5b8bb303e2f0590bb248b5f2 - https://github.com/alphagov/digitalmarketplace-supplier-frontend/commit/e761c1ce655137fed11f730da4e0a2b1893dee91 --- app/main/forms.py | 116 +++++++++++++++++++++++++++------------------- 1 file changed, 68 insertions(+), 48 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 614698989..5555f8506 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1,5 +1,6 @@ import re import pytz +import weakref from flask_wtf import FlaskForm as Form from datetime import datetime, timedelta @@ -98,6 +99,12 @@ def email_address(label='Email address', gov_user=True): return EmailField(label, validators) +def strip_whitespace(value): + if value is not None and hasattr(value, 'strip'): + return value.strip() + return value + + class UKMobileNumber(TelField): def pre_validate(self, form): try: @@ -154,7 +161,20 @@ def organisation_type(): ) -class LoginForm(Form): +class StripWhitespaceForm(Form): + class Meta: + def bind_field(self, form, unbound_field, options): + # FieldList simply doesn't support filters. + # @see: https://github.com/wtforms/wtforms/issues/148 + no_filter_fields = (FieldList, PasswordField) + filters = [strip_whitespace] if not issubclass(unbound_field.field_class, no_filter_fields) else [] + filters += unbound_field.kwargs.get('filters', []) + bound = unbound_field.bind(form=form, filters=filters, **options) + bound.get_form = weakref.ref(form) # GC won't collect the form if we don't use a weakref + return bound + + +class LoginForm(StripWhitespaceForm): email_address = StringField('Email address', validators=[ Length(min=5, max=255), DataRequired(message='Can’t be empty'), @@ -165,7 +185,7 @@ class LoginForm(Form): ]) -class RegisterUserForm(Form): +class RegisterUserForm(StripWhitespaceForm): name = StringField('Full name', validators=[DataRequired(message='Can’t be empty')]) email_address = email_address() @@ -175,7 +195,7 @@ class RegisterUserForm(Form): auth_type = HiddenField('auth_type', default='sms_auth') -class RegisterUserFromInviteForm(Form): +class RegisterUserFromInviteForm(StripWhitespaceForm): def __init__(self, invited_user): super().__init__( service=invited_user['service'], @@ -198,7 +218,7 @@ class RegisterUserFromInviteForm(Form): raise ValidationError('Can’t be empty') -class PermissionsForm(Form): +class PermissionsForm(StripWhitespaceForm): send_messages = BooleanField("Send messages from existing templates") manage_templates = BooleanField("Add and edit templates") manage_service = BooleanField("Modify this service and its team") @@ -225,7 +245,7 @@ class InviteUserForm(PermissionsForm): raise ValidationError("You can’t send an invitation to yourself") -class TwoFactorForm(Form): +class TwoFactorForm(StripWhitespaceForm): def __init__(self, validate_code_func, *args, **kwargs): ''' Keyword arguments: @@ -242,15 +262,15 @@ class TwoFactorForm(Form): raise ValidationError(reason) -class EmailNotReceivedForm(Form): +class EmailNotReceivedForm(StripWhitespaceForm): email_address = email_address() -class TextNotReceivedForm(Form): +class TextNotReceivedForm(StripWhitespaceForm): mobile_number = international_phone_number() -class RenameServiceForm(Form): +class RenameServiceForm(StripWhitespaceForm): name = StringField( u'Service name', validators=[ @@ -258,7 +278,7 @@ class RenameServiceForm(Form): ]) -class CreateServiceForm(Form): +class CreateServiceForm(StripWhitespaceForm): name = StringField( u'What’s your service called?', validators=[ @@ -267,11 +287,11 @@ class CreateServiceForm(Form): organisation_type = organisation_type() -class OrganisationTypeForm(Form): +class OrganisationTypeForm(StripWhitespaceForm): organisation_type = organisation_type() -class FreeSMSAllowance(Form): +class FreeSMSAllowance(StripWhitespaceForm): free_sms_allowance = IntegerField( 'Numbers of text message fragments per year', validators=[ @@ -280,7 +300,7 @@ class FreeSMSAllowance(Form): ) -class ConfirmPasswordForm(Form): +class ConfirmPasswordForm(StripWhitespaceForm): def __init__(self, validate_password_func, *args, **kwargs): self.validate_password_func = validate_password_func super(ConfirmPasswordForm, self).__init__(*args, **kwargs) @@ -292,7 +312,7 @@ class ConfirmPasswordForm(Form): raise ValidationError('Invalid password') -class BaseTemplateForm(Form): +class BaseTemplateForm(StripWhitespaceForm): name = StringField( u'Template name', validators=[DataRequired(message="Can’t be empty")]) @@ -341,15 +361,15 @@ class LetterTemplateForm(EmailTemplateForm): ) -class ForgotPasswordForm(Form): +class ForgotPasswordForm(StripWhitespaceForm): email_address = email_address(gov_user=False) -class NewPasswordForm(Form): +class NewPasswordForm(StripWhitespaceForm): new_password = password() -class ChangePasswordForm(Form): +class ChangePasswordForm(StripWhitespaceForm): def __init__(self, validate_password_func, *args, **kwargs): self.validate_password_func = validate_password_func super(ChangePasswordForm, self).__init__(*args, **kwargs) @@ -362,16 +382,16 @@ class ChangePasswordForm(Form): raise ValidationError('Invalid password') -class CsvUploadForm(Form): +class CsvUploadForm(StripWhitespaceForm): file = FileField('Add recipients', validators=[DataRequired( message='Please pick a file'), CsvFileValidator()]) -class ChangeNameForm(Form): +class ChangeNameForm(StripWhitespaceForm): new_name = StringField(u'Your name') -class ChangeEmailForm(Form): +class ChangeEmailForm(StripWhitespaceForm): def __init__(self, validate_email_func, *args, **kwargs): self.validate_email_func = validate_email_func super(ChangeEmailForm, self).__init__(*args, **kwargs) @@ -384,11 +404,11 @@ class ChangeEmailForm(Form): raise ValidationError("The email address is already in use") -class ChangeMobileNumberForm(Form): +class ChangeMobileNumberForm(StripWhitespaceForm): mobile_number = international_phone_number() -class ConfirmMobileNumberForm(Form): +class ConfirmMobileNumberForm(StripWhitespaceForm): def __init__(self, validate_code_func, *args, **kwargs): self.validate_code_func = validate_code_func super(ConfirmMobileNumberForm, self).__init__(*args, **kwargs) @@ -401,7 +421,7 @@ class ConfirmMobileNumberForm(Form): raise ValidationError(msg) -class ChooseTimeForm(Form): +class ChooseTimeForm(StripWhitespaceForm): def __init__(self, *args, **kwargs): super(ChooseTimeForm, self).__init__(*args, **kwargs) @@ -421,7 +441,7 @@ class ChooseTimeForm(Form): ) -class CreateKeyForm(Form): +class CreateKeyForm(StripWhitespaceForm): def __init__(self, existing_key_names=[], *args, **kwargs): self.existing_key_names = [x.lower() for x in existing_key_names] super(CreateKeyForm, self).__init__(*args, **kwargs) @@ -442,7 +462,7 @@ class CreateKeyForm(Form): raise ValidationError('A key with this name already exists') -class SupportType(Form): +class SupportType(StripWhitespaceForm): support_type = RadioField( 'How can we help you?', choices=[ @@ -453,7 +473,7 @@ class SupportType(Form): ) -class Feedback(Form): +class Feedback(StripWhitespaceForm): name = StringField('Name') email_address = StringField('Email address') feedback = TextAreaField('Your message', validators=[DataRequired(message="Can’t be empty")]) @@ -463,7 +483,7 @@ class Problem(Feedback): email_address = email_address(label='Email address', gov_user=False) -class Triage(Form): +class Triage(StripWhitespaceForm): severe = RadioField( 'Is it an emergency?', choices=[ @@ -474,7 +494,7 @@ class Triage(Form): ) -class RequestToGoLiveForm(Form): +class RequestToGoLiveForm(StripWhitespaceForm): mou = RadioField( ( 'Has your organisation accepted the GOV.UK Notify data sharing and financial ' @@ -513,16 +533,16 @@ class RequestToGoLiveForm(Form): ) -class ProviderForm(Form): +class ProviderForm(StripWhitespaceForm): priority = IntegerField('Priority', [validators.NumberRange(min=1, max=100, message="Must be between 1 and 100")]) -class ServiceReplyToEmailForm(Form): +class ServiceReplyToEmailForm(StripWhitespaceForm): email_address = email_address(label='Email reply to address') is_default = BooleanField("Make this email address the default") -class ServiceSmsSenderForm(Form): +class ServiceSmsSenderForm(StripWhitespaceForm): sms_sender = StringField( 'Text message sender', validators=[ @@ -537,11 +557,11 @@ class ServiceSmsSenderForm(Form): raise ValidationError('Use letters and numbers only') -class ServiceEditInboundNumberForm(Form): +class ServiceEditInboundNumberForm(StripWhitespaceForm): is_default = BooleanField("Make this text message sender the default") -class ServiceLetterContactBlockForm(Form): +class ServiceLetterContactBlockForm(StripWhitespaceForm): letter_contact_block = TextAreaField( validators=[ DataRequired(message="Can’t be empty"), @@ -558,7 +578,7 @@ class ServiceLetterContactBlockForm(Form): ) -class ServiceBrandingOrg(Form): +class ServiceBrandingOrg(StripWhitespaceForm): def __init__(self, organisations=[], *args, **kwargs): self.organisation.choices = organisations @@ -585,7 +605,7 @@ class ServiceBrandingOrg(Form): ) -class ServiceSelectOrg(Form): +class ServiceSelectOrg(StripWhitespaceForm): def __init__(self, organisations=[], *args, **kwargs): self.organisation.choices = organisations @@ -599,7 +619,7 @@ class ServiceSelectOrg(Form): ) -class ServiceManageOrg(Form): +class ServiceManageOrg(StripWhitespaceForm): name = StringField('Name') @@ -607,7 +627,7 @@ class ServiceManageOrg(Form): file = FileField_wtf('Upload a PNG logo', validators=[FileAllowed(['png'], 'PNG Images only!')]) -class LetterBranding(Form): +class LetterBranding(StripWhitespaceForm): def __init__(self, choices=[], *args, **kwargs): super().__init__(*args, **kwargs) @@ -621,7 +641,7 @@ class LetterBranding(Form): ) -class Whitelist(Form): +class Whitelist(StripWhitespaceForm): def populate(self, email_addresses, phone_numbers): for form_field, existing_whitelist in ( @@ -659,13 +679,13 @@ class Whitelist(Form): ) -class DateFilterForm(Form): +class DateFilterForm(StripWhitespaceForm): start_date = DateField("Start Date", [validators.optional()]) end_date = DateField("End Date", [validators.optional()]) include_from_test_key = BooleanField("Include test keys", default="checked", false_values={"N"}) -class ChooseTemplateType(Form): +class ChooseTemplateType(StripWhitespaceForm): template_type = RadioField( 'What kind of template do you want to add?', @@ -685,17 +705,17 @@ class ChooseTemplateType(Form): ]) -class SearchTemplatesForm(Form): +class SearchTemplatesForm(StripWhitespaceForm): search = SearchField('Search by name') -class SearchNotificationsForm(Form): +class SearchNotificationsForm(StripWhitespaceForm): to = SearchField('Search by phone number or email address') -class PlaceholderForm(Form): +class PlaceholderForm(StripWhitespaceForm): pass @@ -704,7 +724,7 @@ class PasswordFieldShowHasContent(StringField): widget = widgets.PasswordInput(hide_value=False) -class ServiceInboundNumberForm(Form): +class ServiceInboundNumberForm(StripWhitespaceForm): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.inbound_number.choices = kwargs['inbound_number_choices'] @@ -717,7 +737,7 @@ class ServiceInboundNumberForm(Form): ) -class ServiceReceiveMessagesCallbackForm(Form): +class ServiceReceiveMessagesCallbackForm(StripWhitespaceForm): url = StringField( "URL", validators=[DataRequired(message='Can’t be empty'), @@ -730,7 +750,7 @@ class ServiceReceiveMessagesCallbackForm(Form): ) -class ServiceDeliveryStatusCallbackForm(Form): +class ServiceDeliveryStatusCallbackForm(StripWhitespaceForm): url = StringField( "URL", validators=[DataRequired(message='Can’t be empty'), @@ -743,7 +763,7 @@ class ServiceDeliveryStatusCallbackForm(Form): ) -class InternationalSMSForm(Form): +class InternationalSMSForm(StripWhitespaceForm): enabled = RadioField( 'Send text messages to international phone numbers', choices=[ @@ -753,7 +773,7 @@ class InternationalSMSForm(Form): ) -class SMSPrefixForm(Form): +class SMSPrefixForm(StripWhitespaceForm): enabled = RadioField( '', choices=[ @@ -791,7 +811,7 @@ def get_placeholder_form_instance( ) -class SetSenderForm(Form): +class SetSenderForm(StripWhitespaceForm): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) From d0d230f11956703ecf9cd404211e693e0df7c412 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 11 Dec 2017 16:09:19 +0000 Subject: [PATCH 2/3] Remove existing whitespace stripping code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since we’re doing this globally, we don’t need to handle it in a custom way for the sign in form (and it’s much nicer encapsulated like this). Also added some more extensive tests in this commit. --- app/main/views/sign_in.py | 2 -- tests/app/main/views/test_register.py | 7 ++++++- tests/app/main/views/test_sign_in.py | 11 ++++++----- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/app/main/views/sign_in.py b/app/main/views/sign_in.py index fc8e9b049..eb77ad7a7 100644 --- a/app/main/views/sign_in.py +++ b/app/main/views/sign_in.py @@ -30,8 +30,6 @@ def sign_in(): return redirect(url_for('main.choose_service')) form = LoginForm() - if form.email_address.data: - form.email_address.data = form.email_address.data.strip() if form.validate_on_submit(): diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index ba5cc3b9e..b5ba1d508 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -39,6 +39,10 @@ def test_logged_in_user_redirects_to_choose_service( '+4407700900460', '+1800-555-555', ]) +@pytest.mark.parametrize('password', [ + 'the quick brown fox', + ' the quick brown fox ', +]) def test_register_creates_new_user_and_redirects_to_continue_page( client, mock_send_verify_code, @@ -48,11 +52,12 @@ def test_register_creates_new_user_and_redirects_to_continue_page( mock_send_verify_email, mock_login, phone_number_to_register_with, + password, ): user_data = {'name': 'Some One Valid', 'email_address': 'notfound@example.gov.uk', 'mobile_number': phone_number_to_register_with, - 'password': 'validPassword!', + 'password': password, 'auth_type': 'sms_auth' } diff --git a/tests/app/main/views/test_sign_in.py b/tests/app/main/views/test_sign_in.py index dc9d4da29..6f44c8d4a 100644 --- a/tests/app/main/views/test_sign_in.py +++ b/tests/app/main/views/test_sign_in.py @@ -77,9 +77,9 @@ def test_logged_in_user_redirects_to_choose_service( assert response.location == url_for('main.choose_service', _external=True) -@pytest.mark.parametrize('email_address', [ - 'valid@example.gov.uk', - ' valid@example.gov.uk ', +@pytest.mark.parametrize('email_address, password', [ + ('valid@example.gov.uk', 'val1dPassw0rd!'), + (' valid@example.gov.uk ', ' val1dPassw0rd! '), ]) def test_process_sms_auth_sign_in_return_2fa_template( client, @@ -89,14 +89,15 @@ def test_process_sms_auth_sign_in_return_2fa_template( mock_get_user_by_email, mock_verify_password, email_address, + password, ): response = client.post( url_for('main.sign_in'), data={ 'email_address': email_address, - 'password': 'val1dPassw0rd!'}) + 'password': password}) assert response.status_code == 302 assert response.location == url_for('.two_factor', _external=True) - mock_verify_password.assert_called_with(api_user_active.id, 'val1dPassw0rd!') + mock_verify_password.assert_called_with(api_user_active.id, password) mock_get_user_by_email.assert_called_with('valid@example.gov.uk') From a8829cd15449634ea3790abf0e6c8d5f69b96d3a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 11 Dec 2017 16:22:37 +0000 Subject: [PATCH 3/3] Make whitespace stripping work for whitelists too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s a bit hacky, but it fixes a potential issue for users. Code adapted from: https://github.com/alphagov/digitalmarketplace-utils/commit/2c34f678ab4a34eaa1c510c91dacfc210343f389 --- app/main/forms.py | 24 ++++++++++++++++++++++-- tests/app/main/views/test_api_keys.py | 4 ++-- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 5555f8506..ea89af61d 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -4,6 +4,7 @@ import weakref from flask_wtf import FlaskForm as Form from datetime import datetime, timedelta +from itertools import chain from notifications_utils.recipients import ( validate_phone_number, @@ -174,6 +175,17 @@ class StripWhitespaceForm(Form): return bound +class StripWhitespaceStringField(StringField): + def __init__(self, label=None, **kwargs): + kwargs['filters'] = tuple(chain( + kwargs.get('filters', ()), + ( + strip_whitespace, + ), + )) + super(StringField, self).__init__(label, **kwargs) + + class LoginForm(StripWhitespaceForm): email_address = StringField('Email address', validators=[ Length(min=5, max=255), @@ -641,6 +653,14 @@ class LetterBranding(StripWhitespaceForm): ) +class EmailFieldInWhitelist(EmailField, StripWhitespaceStringField): + pass + + +class InternationalPhoneNumberInWhitelist(InternationalPhoneNumber, StripWhitespaceStringField): + pass + + class Whitelist(StripWhitespaceForm): def populate(self, email_addresses, phone_numbers): @@ -652,7 +672,7 @@ class Whitelist(StripWhitespaceForm): form_field[index].data = value email_addresses = FieldList( - EmailField( + EmailFieldInWhitelist( '', validators=[ Optional(), @@ -666,7 +686,7 @@ class Whitelist(StripWhitespaceForm): ) phone_numbers = FieldList( - InternationalPhoneNumber( + InternationalPhoneNumberInWhitelist( '', validators=[ Optional() diff --git a/tests/app/main/views/test_api_keys.py b/tests/app/main/views/test_api_keys.py index b5e11f251..a874eacad 100644 --- a/tests/app/main/views/test_api_keys.py +++ b/tests/app/main/views/test_api_keys.py @@ -351,8 +351,8 @@ def test_should_update_whitelist( service_id = str(uuid.uuid4()) data = OrderedDict([ ('email_addresses-1', 'test@example.com'), - ('email_addresses-3', 'test@example.com'), - ('phone_numbers-0', '07900900000'), + ('email_addresses-3', ' test@example.com '), + ('phone_numbers-0', '07900900000 '), ('phone_numbers-2', '+1800-555-555'), ])