From d7f68bab42cb8995a9d158c5221219d4d628bbe6 Mon Sep 17 00:00:00 2001 From: Chris Wagner Date: Tue, 25 Jul 2023 19:52:05 -0700 Subject: [PATCH] Fix login and unified signin templates to send CSRF tokens. (#825) Add additional tests. The oauthstart view needs to be decorated with @unauth_csrf. Turn on complete CSRF support in view_scaffold since we really don't test CSRF enough. --- CHANGES.rst | 3 +- docs/patterns.rst | 2 +- flask_security/oauth_glue.py | 4 +- .../templates/security/login_user.html | 7 +++- .../templates/security/us_signin.html | 5 ++- tests/test_oauthglue.py | 37 ++++++++++++++++++- tests/test_utils.py | 30 +++++++++------ tests/view_scaffold.py | 4 +- 8 files changed, 73 insertions(+), 19 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index f124978e..e43c44e0 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -25,8 +25,9 @@ Fixes - (:issue:`281`) Reset password can be exploited and other OWASP improvements. - (:pr:`817`) Confirmation can be exploited and other OWASP improvements. - (:pr:`819`) Convert to pyproject.toml, build, remove setup. -- (:pr:`xxx`) the tf_validity feature now ONLY sets a cookie - and the token is no longer +- (:pr:`823`) the tf_validity feature now ONLY sets a cookie - and the token is no longer returned as part of a JSON response. +- (:pr:`xxx`) Fix login/unified signin templates to properly send CSRF token. Add more tests. Backwards Compatibility Concerns +++++++++++++++++++++++++++++++++ diff --git a/docs/patterns.rst b/docs/patterns.rst index 4af65afa..5544230e 100644 --- a/docs/patterns.rst +++ b/docs/patterns.rst @@ -251,7 +251,7 @@ Note that we use the header name ``X-CSRF-Token`` as that is one of the default headers configured in Flask-WTF (*WTF_CSRF_HEADERS*) To protect your application's endpoints (that presumably are not using Flask forms), -you need to enable CSRF as described in the FlaskWTF `documentation `_: :: +you need to enable CSRF as described in the FlaskWTF `documentation `_: :: flask_wtf.CSRFProtect(app) diff --git a/flask_security/oauth_glue.py b/flask_security/oauth_glue.py index 444c106f..b814b562 100644 --- a/flask_security/oauth_glue.py +++ b/flask_security/oauth_glue.py @@ -4,7 +4,7 @@ Class and methods to glue our login path with authlib for to support 'social' auth. - :copyright: (c) 2022-2022 by J. Christopher Wagner (jwag). + :copyright: (c) 2022-2023 by J. Christopher Wagner (jwag). :license: MIT, see LICENSE for more details. """ @@ -23,6 +23,7 @@ from flask import abort, after_this_request, redirect, request +from .decorators import unauth_csrf from .proxies import _security from .utils import ( config_value as cv, @@ -73,6 +74,7 @@ def google_fetch_identity( return "email", profile["email"] +@unauth_csrf() def oauthstart(name: str) -> "ResponseValue": """View to start an oauth authentication. Name is a pre-registered oauth provider. diff --git a/flask_security/templates/security/login_user.html b/flask_security/templates/security/login_user.html index 0991a68b..2d254921 100644 --- a/flask_security/templates/security/login_user.html +++ b/flask_security/templates/security/login_user.html @@ -21,7 +21,7 @@

{{ _fsdomain('Login') }}

{{ _fsdomain("Use WebAuthn to Sign In") }}

-
+
@@ -31,8 +31,11 @@

{{ _fsdomain("Use WebAuthn to Sign In") }}

{{ _fsdomain("Use Social Oauth to Sign In") }}

{% for provider in security.oauthglue.provider_names %}
-
+ + {% if csrf_token is defined %} + + {% endif %}
{% endfor %} diff --git a/flask_security/templates/security/us_signin.html b/flask_security/templates/security/us_signin.html index b29a43cb..bd9694db 100644 --- a/flask_security/templates/security/us_signin.html +++ b/flask_security/templates/security/us_signin.html @@ -34,8 +34,11 @@

{{ _fsdomain("Use WebAuthn to Sign In") }}

{{ _fsdomain("Use Social Oauth to Sign In") }}

{% for provider in security.oauthglue.provider_names %}
-
+ + {% if csrf_token is defined %} + + {% endif %}
{% endfor %} diff --git a/tests/test_oauthglue.py b/tests/test_oauthglue.py index dee0f663..82542e1a 100644 --- a/tests/test_oauthglue.py +++ b/tests/test_oauthglue.py @@ -4,7 +4,7 @@ Oauth glue tests - oauthglue is a very thin shim between FS and authlib - :copyright: (c) 2022-2022 by J. Christopher Wagner (jwag). + :copyright: (c) 2022-2023 by J. Christopher Wagner (jwag). :license: MIT, see LICENSE for more details. """ @@ -13,10 +13,13 @@ from urllib.parse import parse_qsl, urlsplit from flask import redirect +from flask_wtf import CSRFProtect from tests.test_utils import ( authenticate, + get_csrf_token, get_form_action, + get_form_input, init_app_with_options, logout, setup_tf_sms, @@ -69,15 +72,24 @@ def register(self, name, **kwargs): @pytest.mark.settings(oauth_enable=True, post_login_view="/post_login") +@pytest.mark.app_settings(wtf_csrf_enabled=True) def test_github(app, sqlalchemy_datastore, get_message): + CSRFProtect(app) init_app_with_options( app, sqlalchemy_datastore, **{"security_args": {"oauth": MockOAuth()}} ) client = app.test_client() response = client.get("/login") github_url = get_form_action(response, 1) + csrf_token = get_form_input(response, field_id="github_csrf_token") + # make sure required CSRF response = client.post(github_url, follow_redirects=False) + assert "The CSRF token is missing" + + response = client.post( + github_url, data=dict(csrf_token=csrf_token), follow_redirects=False + ) assert "/whatever" in response.location response = client.get("/login/oauthresponse/github", follow_redirects=False) @@ -88,6 +100,23 @@ def test_github(app, sqlalchemy_datastore, get_message): assert response.status_code == 200 +@pytest.mark.settings( + oauth_enable=True, post_login_view="/post_login", csrf_ignore_unauth_endpoints=True +) +@pytest.mark.app_settings(wtf_csrf_enabled=True, wtf_csrf_check_default=False) +def test_github_nocsrf(app, sqlalchemy_datastore, get_message): + # Test if ignore_unauth_endpoints is true - doesn't require CSRF + CSRFProtect(app) + init_app_with_options( + app, sqlalchemy_datastore, **{"security_args": {"oauth": MockOAuth()}} + ) + client = app.test_client() + response = client.get("/login") + github_url = get_form_action(response, 1) + response = client.post(github_url, follow_redirects=False) + assert "/whatever" in response.location + + @pytest.mark.settings(oauth_enable=True, post_login_view="/post_login") def test_outside_register(app, sqlalchemy_datastore, get_message): def myoauth_fetch_identity(oauth, token): @@ -188,14 +217,20 @@ def test_tf(app, sqlalchemy_datastore, get_message): redirect_behavior="spa", login_error_view="/login-error", post_login_view="/post-login", + csrf_ignore_unauth_endpoints=False, ) +@pytest.mark.app_settings(wtf_csrf_enabled=True) def test_spa(app, sqlalchemy_datastore, get_message): + CSRFProtect(app) headers = {"Accept": "application/json", "Content-Type": "application/json"} init_app_with_options( app, sqlalchemy_datastore, **{"security_args": {"oauth": MockOAuth()}} ) client = app.test_client() + csrf_token = get_csrf_token(client) + headers["X-CSRF-Token"] = csrf_token + response = client.post("/login/oauthstart/github", headers=headers) assert "/whatever" in response.location redirect_url = urllib.parse.urlsplit(urllib.parse.unquote(response.location)) diff --git a/tests/test_utils.py b/tests/test_utils.py index fe046e66..e1587359 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -1,10 +1,10 @@ """ - utils - ~~~~~ + test_utils + ~~~~~~~~~~ Test utils - :copyright: (c) 2019-2022 by J. Christopher Wagner (jwag). + :copyright: (c) 2019-2023 by J. Christopher Wagner (jwag). :license: MIT, see LICENSE for more details. """ from contextlib import contextmanager @@ -42,11 +42,11 @@ def authenticate( def json_authenticate(client, email="matt@lp.com", password="password", endpoint=None): - data = f'{{"email": "{email}", "password": "{password}"}}' + data = dict(email=email, password=password) # Get auth token always ep = endpoint or "/login?include_auth_token" - return client.post(ep, content_type="application/json", data=data) + return client.post(ep, content_type="application/json", json=data) def is_authenticated(client, get_message): @@ -165,6 +165,19 @@ def get_form_action(response, ordinal=0): return matcher[ordinal] +def get_form_input(response, field_id): + # return value of field with the id == field_id or None if not found + rex = f']*value="([^"]*)">' + matcher = re.findall( + rex, + response.data.decode("utf-8"), + re.IGNORECASE | re.DOTALL, + ) + if matcher: + return matcher[0] + return None + + def check_xlation(app, locale): """Return True if locale is loaded""" with app.test_request_context(): @@ -251,12 +264,7 @@ def get_num_queries(datastore): return None if datastore doesn't support this. """ if is_sqlalchemy(datastore): - try: - # Flask-SQLAlachemy >= 3.0.0 - from flask_sqlalchemy.record_queries import get_recorded_queries - except ImportError: - # Flask-SQLAlchemy < 3.0.0 - from flask_sqlalchemy import get_debug_queries as get_recorded_queries + from flask_sqlalchemy.record_queries import get_recorded_queries return len(get_recorded_queries()) return None diff --git a/tests/view_scaffold.py b/tests/view_scaffold.py index 88adb966..90cf92d0 100644 --- a/tests/view_scaffold.py +++ b/tests/view_scaffold.py @@ -1,4 +1,4 @@ -# :copyright: (c) 2019-2022 by J. Christopher Wagner (jwag). +# :copyright: (c) 2019-2023 by J. Christopher Wagner (jwag). # :license: MIT, see LICENSE for more details. """ @@ -27,6 +27,7 @@ from flask import Flask, flash, render_template_string, request, session from flask_sqlalchemy import SQLAlchemy +from flask_wtf import CSRFProtect from flask_security import ( MailUtil, @@ -152,6 +153,7 @@ def origin(self) -> str: ): app.config[ev] = _find_bool(os.environ.get(ev)) + CSRFProtect(app) # Create database models and hook up. app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False db = SQLAlchemy(app)