From 3e6d9a564bb77907be52cc8d234be69b33849433 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 4 May 2020 12:27:51 +0100 Subject: [PATCH] Add interstitial page before using email auth token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some email clients will pre-fetch links in emails to check whether they’re safe. This has the unfortunate side effect of claiming the token that’s in the link. Long term, we don’t want to let the link be used multiple times, because this reduces how secure it is (eg someone with access to your browser history could re-use the link even if you’d signed out). Instead, this commit adds an extra page which is served when the user clicks the link from the email. This page includes a form which submits to the actual URL that uses the token, thereby not claiming the token as soon as the page is loaded. For convenience, this page also includes some Javascript which clicks the link on the user’s behalf. If the user has Javascript turned off they will see the link and can click it themselves. This is going on the assumption that whatever the email clients are doing when prefetching the link doesn’t involve running any Javascript. This Javascript is inlined so that: - it is run as fast as possible - it’s more resilient – even if our assets domain is unreachable or the connection is interrupted, it will still run --- app/main/views/two_factor.py | 7 ++- app/navigation.py | 4 ++ .../views/email-link-interstitial.html | 25 +++++++++ tests/app/main/views/test_two_factor.py | 56 ++++++++++++++++--- 4 files changed, 82 insertions(+), 10 deletions(-) create mode 100644 app/templates/views/email-link-interstitial.html diff --git a/app/main/views/two_factor.py b/app/main/views/two_factor.py index 1e2aadcad..e9140e8aa 100644 --- a/app/main/views/two_factor.py +++ b/app/main/views/two_factor.py @@ -28,7 +28,12 @@ def two_factor_email_sent(): ) -@main.route('/email-auth/', methods=['GET', 'POST']) +@main.route('/email-auth/', methods=['GET']) +def two_factor_email_interstitial(token): + return render_template('views/email-link-interstitial.html') + + +@main.route('/email-auth/', methods=['POST']) def two_factor_email(token): if current_user.is_authenticated: return redirect_when_logged_in(platform_admin=current_user.platform_admin) diff --git a/app/navigation.py b/app/navigation.py index 210354599..acf2e9406 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -118,6 +118,7 @@ class HeaderNavigation(Navigation): 'two_factor', 'two_factor_email', 'two_factor_email_sent', + 'two_factor_email_interstitial', 'verify', 'verify_email', }, @@ -637,6 +638,7 @@ class MainNavigation(Navigation): 'two_factor', 'two_factor_email', 'two_factor_email_sent', + 'two_factor_email_interstitial', 'update_email_branding', 'update_letter_branding', 'usage_for_all_services', @@ -939,6 +941,7 @@ class CaseworkNavigation(Navigation): 'two_factor', 'two_factor_email', 'two_factor_email_sent', + 'two_factor_email_interstitial', 'update_email_branding', 'update_letter_branding', 'usage', @@ -1235,6 +1238,7 @@ class OrgNavigation(Navigation): 'two_factor', 'two_factor_email', 'two_factor_email_sent', + 'two_factor_email_interstitial', 'update_email_branding', 'update_letter_branding', 'upload_contact_list', diff --git a/app/templates/views/email-link-interstitial.html b/app/templates/views/email-link-interstitial.html new file mode 100644 index 000000000..a0b41c965 --- /dev/null +++ b/app/templates/views/email-link-interstitial.html @@ -0,0 +1,25 @@ +{% extends "withoutnav_template.html" %} +{% from "components/page-header.html" import page_header %} +{% from "components/page-footer.html" import page_footer %} + +{% block per_page_title %} + Sign in +{% endblock %} + +{% block maincolumn_content %} + +
+ + {{ page_header('Sign in') }} + +
+ {{ page_footer('Continue to dashboard') }} +
+ +
+ + + +{% endblock %} diff --git a/tests/app/main/views/test_two_factor.py b/tests/app/main/views/test_two_factor.py index 5b4dd45b3..70c480242 100644 --- a/tests/app/main/views/test_two_factor.py +++ b/tests/app/main/views/test_two_factor.py @@ -253,9 +253,48 @@ def test_two_factor_should_activate_pending_user( assert mock_activate_user.called -@pytest.mark.parametrize('http_method', ( - 'get', 'post', +@pytest.mark.parametrize('extra_args, expected_encoded_next_arg', ( + ({}, ''), + ({'next': 'https://example.com'}, '?next=https%3A%2F%2Fexample.com') )) +def test_valid_two_factor_email_link_shows_interstitial( + client_request, + valid_token, + mocker, + extra_args, + expected_encoded_next_arg, +): + mock_check_code = mocker.patch('app.user_api_client.check_verify_code') + encoded_token = valid_token.replace('%2E', '.') + token_url = url_for( + 'main.two_factor_email_interstitial', + token=encoded_token, + **extra_args + ) + + # This must match the URL we put in the emails + assert token_url == f'/email-auth/{encoded_token}{expected_encoded_next_arg}' + + client_request.logout() + page = client_request.get_url(token_url) + + assert normalize_spaces(page.select_one('main .js-hidden').text) == ( + 'Sign in ' + 'Continue to dashboard' + ) + + form = page.select_one('form') + expected_form_id = 'use-email-auth' + assert 'action' not in form + assert form['method'] == 'post' + assert form['id'] == expected_form_id + assert page.select_one('main script').text.strip() == ( + f'document.getElementById("{expected_form_id}").submit();' + ) + + assert mock_check_code.called is False + + def test_valid_two_factor_email_link_logs_in_user( client, valid_token, @@ -263,11 +302,10 @@ def test_valid_two_factor_email_link_logs_in_user( mock_get_services_with_one_service, mocker, mock_create_event, - http_method, ): mocker.patch('app.user_api_client.check_verify_code', return_value=(True, '')) - response = getattr(client, http_method)( + response = client.post( url_for_endpoint_with_token('main.two_factor_email', token=valid_token), ) @@ -284,7 +322,7 @@ def test_two_factor_email_link_has_expired( ): with set_config(app_, 'EMAIL_2FA_EXPIRY_SECONDS', -1): - response = client.get( + response = client.post( url_for_endpoint_with_token('main.two_factor_email', token=valid_token), follow_redirects=True, ) @@ -300,7 +338,7 @@ def test_two_factor_email_link_is_invalid( client ): token = 12345 - response = client.get( + response = client.post( url_for('main.two_factor_email', token=token), follow_redirects=True ) @@ -320,7 +358,7 @@ def test_two_factor_email_link_is_already_used( ): mocker.patch('app.user_api_client.check_verify_code', return_value=(False, 'Code has expired')) - response = client.get( + response = client.post( url_for_endpoint_with_token('main.two_factor_email', token=valid_token), follow_redirects=True ) @@ -340,7 +378,7 @@ def test_two_factor_email_link_when_user_is_locked_out( ): mocker.patch('app.user_api_client.check_verify_code', return_value=(False, 'Code not found')) - response = client.get( + response = client.post( url_for_endpoint_with_token('main.two_factor_email', token=valid_token), follow_redirects=True ) @@ -356,7 +394,7 @@ def test_two_factor_email_link_used_when_user_already_logged_in( logged_in_client, valid_token ): - response = logged_in_client.get( + response = logged_in_client.post( url_for_endpoint_with_token('main.two_factor_email', token=valid_token) ) assert response.status_code == 302