From 2124821e0078dda16e51c2a668558a331ef49156 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 15 Feb 2016 13:05:25 +0000 Subject: [PATCH 1/4] =?UTF-8?q?Don=E2=80=99t=20allow=20autocomplete=20on?= =?UTF-8?q?=20register=20page?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a potential attack vector which was highlighted by the pen test. Setting autocomplete to `nope` (or any random string) is the most comprehensive way of telling browsers not to autocomplete a form according to: https://developer.mozilla.org/en-US/docs/Web/Security/Securing_your_site/Turning_off_form_autocompletion --- app/templates/views/register.html | 2 +- app/templates/views/signin.html | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/templates/views/register.html b/app/templates/views/register.html index 01e86f1ed..dd13e1014 100644 --- a/app/templates/views/register.html +++ b/app/templates/views/register.html @@ -14,7 +14,7 @@ Create an account – GOV.UK Notify

If you've used GOV.UK Notify before, sign in to your account.

-
+ {{ textbox(form.name, width='3-4') }} {{ textbox(form.email_address, hint="Your email address must end in .gov.uk", width='3-4') }} {{ textbox(form.mobile_number, width='3-4') }} diff --git a/app/templates/views/signin.html b/app/templates/views/signin.html index 44906ff98..d08778e0e 100644 --- a/app/templates/views/signin.html +++ b/app/templates/views/signin.html @@ -14,7 +14,7 @@

If you do not have an account, you can register for one now.

- + {{ textbox(form.email_address) }} {{ textbox(form.password) }} {{ page_footer("Continue", secondary_link=url_for('.forgot_password'), secondary_link_text="Forgotten password?") }} From 6b4ede629cfb938cac93aabaa1b81d7920d4a63e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 15 Feb 2016 13:13:57 +0000 Subject: [PATCH 2/4] Use correct HTML 5 input types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These give devices a hint (although don’t mandate them) to use a numeric keypad, or a keypad with the `@` symbol visible when entering phone numbers or email addresses. --- app/main/forms.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index a5e7f739c..d97784863 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -9,6 +9,7 @@ from wtforms import ( FileField, RadioField ) +from wtforms.fields.html5 import EmailField, TelField from wtforms.validators import DataRequired, Email, Length, Regexp from app.main.validators import Blacklist, CsvFileValidator @@ -23,14 +24,14 @@ from app.main.utils import ( def email_address(): gov_uk_email \ = "(^[^@^\\s]+@[^@^\\.^\\s]+(\\.[^@^\\.^\\s]*)*.gov.uk)" - return StringField('Email address', validators=[ + return EmailField('Email address', validators=[ Length(min=5, max=255), DataRequired(message='Email cannot be empty'), Email(message='Enter a valid email address'), Regexp(regex=gov_uk_email, message='Enter a gov.uk email address')]) -class UKMobileNumber(StringField): +class UKMobileNumber(TelField): def pre_validate(self, form): try: From 6a39c8e1879161d13d61d1718ce7f6f755ba4aa8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 15 Feb 2016 13:14:22 +0000 Subject: [PATCH 3/4] Use typographic quotes http://smartquotesforsmartpeople.com --- app/templates/views/register.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/register.html b/app/templates/views/register.html index dd13e1014..bceb246ac 100644 --- a/app/templates/views/register.html +++ b/app/templates/views/register.html @@ -12,7 +12,7 @@ Create an account – GOV.UK Notify

Create an account

-

If you've used GOV.UK Notify before, sign in to your account.

+

If you’ve used GOV.UK Notify before, sign in to your account.

{{ textbox(form.name, width='3-4') }} From e0e445c520413e5d880901c34edd90734d0cf45e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 17 Feb 2016 10:24:32 +0000 Subject: [PATCH 4/4] Stop enumeration of email addresses via forgot pw MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://www.pivotaltracker.com/story/show/113840073 Previously the forgot password page would give an error if you entered an email address which didn’t belong to an account. This would allow a potential attacker to know which email addresses were registered. This commit changes the response to always be the same, whether or not the email address exists. Also, this is a good read about the dangers of asserting whether a mocked method was called: http://engineeringblog.yelp.com/2015/02/assert_called_once-threat-or-menace.html --- app/main/views/forgot_password.py | 3 +- tests/app/main/views/test_forgot_password.py | 32 +++++++++++++++++--- tests/conftest.py | 14 ++++++++- 3 files changed, 42 insertions(+), 7 deletions(-) diff --git a/app/main/views/forgot_password.py b/app/main/views/forgot_password.py index ba4c3d67d..800474200 100644 --- a/app/main/views/forgot_password.py +++ b/app/main/views/forgot_password.py @@ -15,6 +15,7 @@ def forgot_password(): users_dao.request_password_reset(user) send_change_password_email(form.email_address.data) return render_template('views/password-reset-sent.html') - flash('There was an error processing your request') + else: + return render_template('views/password-reset-sent.html') return render_template('views/forgot-password.html', form=form) diff --git a/tests/app/main/views/test_forgot_password.py b/tests/app/main/views/test_forgot_password.py index 65b9f55ee..7789b4197 100644 --- a/tests/app/main/views/test_forgot_password.py +++ b/tests/app/main/views/test_forgot_password.py @@ -9,11 +9,13 @@ def test_should_render_forgot_password(app_): in response.get_data(as_text=True) -def test_should_redirect_to_password_reset_sent_and_state_updated(app_, - api_user_active, - mock_get_user_by_email, - mock_update_user, - mock_send_email): +def test_should_redirect_to_password_reset_sent_and_state_updated( + app_, + api_user_active, + mock_get_user_by_email, + mock_update_user, + mock_send_email +): with app_.test_request_context(): response = app_.test_client().post( url_for('.forgot_password'), @@ -22,3 +24,23 @@ def test_should_redirect_to_password_reset_sent_and_state_updated(app_, assert ( 'You have been sent an email containing a link' ' to reset your password.') in response.get_data(as_text=True) + assert mock_send_email.call_count == 1 + + +def test_should_redirect_to_password_reset_sent_for_non_existant_email_address( + app_, + api_user_active, + mock_dont_get_user_by_email, + mock_update_user, + mock_send_email +): + with app_.test_request_context(): + response = app_.test_client().post( + url_for('.forgot_password'), + data={'email_address': 'nope@example.gov.uk'}) + assert response.status_code == 200 + assert ( + 'You have been sent an email containing a link' + ' to reset your password.') in response.get_data(as_text=True) + mock_dont_get_user_by_email.assert_called_once_with('nope@example.gov.uk') + assert not mock_send_email.called diff --git a/tests/conftest.py b/tests/conftest.py index 3ede76035..d53b1e72e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -41,7 +41,7 @@ def mock_send_sms(request, mocker): @pytest.fixture(scope='function') def mock_send_email(request, mocker): - return mocker.patch("app.notifications_api_client.send_email") + return mocker.patch("app.notifications_api_client.send_email", autospec=True) @pytest.fixture(scope='function') @@ -287,6 +287,18 @@ def mock_get_user_by_email(mocker, api_user_active): return mocker.patch('app.user_api_client.get_user_by_email', side_effect=_get_user) +@pytest.fixture(scope='function') +def mock_dont_get_user_by_email(mocker): + + def _get_user(email_address): + return None + return mocker.patch( + 'app.user_api_client.get_user_by_email', + side_effect=_get_user, + autospec=True + ) + + @pytest.fixture(scope='function') def mock_get_user_by_email_request_password_reset(mocker, api_user_request_password_reset): return mocker.patch(