From 4c19e19a019f1c98226f391f821824c3027261b8 Mon Sep 17 00:00:00 2001 From: Shahar Epstein Date: Sat, 8 Jun 2024 22:53:04 +0300 Subject: [PATCH] Add CSRF protection to "/logout" --- .../auth_manager/security_manager/override.py | 17 +++++++++++++++++ airflow/www/static/css/bootstrap-theme.css | 18 +++++++++++++++--- airflow/www/static/css/material-icons.css | 4 +++- .../www/templates/appbuilder/navbar_right.html | 7 ++++++- newsfragments/40145.significant.rst | 5 +++++ .../test_role_and_permission_endpoint.py | 2 ++ tests/www/views/test_session.py | 4 ++-- 7 files changed, 50 insertions(+), 7 deletions(-) create mode 100644 newsfragments/40145.significant.rst diff --git a/airflow/providers/fab/auth_manager/security_manager/override.py b/airflow/providers/fab/auth_manager/security_manager/override.py index e617b8e84687c..56336b4fb2950 100644 --- a/airflow/providers/fab/auth_manager/security_manager/override.py +++ b/airflow/providers/fab/auth_manager/security_manager/override.py @@ -57,8 +57,10 @@ AuthOAuthView, AuthOIDView, AuthRemoteUserView, + AuthView, RegisterUserModelView, ) +from flask_appbuilder.views import expose from flask_babel import lazy_gettext from flask_jwt_extended import JWTManager, current_user as current_user_jwt from flask_login import LoginManager @@ -123,6 +125,21 @@ MAX_NUM_DATABASE_USER_SESSIONS = 50000 +# The following class patches the logout method within AuthView, so it only supports POST method +# to make CSRF protection effective. You could remove the patch and configure it when it is supported +# natively by Flask-AppBuilder (https://github.com/dpgaspar/Flask-AppBuilder/issues/2248) + + +class _ModifiedAuthView(AuthView): + @expose("/logout/", methods=["POST"]) + def logout(self): + return super().logout() + + +for auth_view in [AuthDBView, AuthLDAPView, AuthOAuthView, AuthOIDView, AuthRemoteUserView]: + auth_view.__bases__ = (_ModifiedAuthView,) + + class FabAirflowSecurityManagerOverride(AirflowSecurityManagerV2): """ This security manager overrides the default AirflowSecurityManager security manager. diff --git a/airflow/www/static/css/bootstrap-theme.css b/airflow/www/static/css/bootstrap-theme.css index 371762d55857e..d409434cc53ab 100644 --- a/airflow/www/static/css/bootstrap-theme.css +++ b/airflow/www/static/css/bootstrap-theme.css @@ -2768,7 +2768,10 @@ tbody.collapse.in { overflow: hidden; background-color: #e5e5e5; } -.dropdown-menu > li > a { + +.dropdown-menu > li > a, +.dropdown-menu > li > form > button +{ display: block; padding: 3px 20px; clear: both; @@ -2776,9 +2779,14 @@ tbody.collapse.in { line-height: 1.428571429; color: #51504f; white-space: nowrap; + width: 100%; + max-width: 100%; + text-align: left; } .dropdown-menu > li > a:hover, -.dropdown-menu > li > a:focus { +.dropdown-menu > li > a:focus, +.dropdown-menu > li > form > button:hover, +.dropdown-menu > li > form > button:focus { text-decoration: none; color: #262626; background-color: #f5f5f5; @@ -3569,13 +3577,17 @@ select[multiple].input-group-sm > .input-group-btn > .btn { box-shadow: none; } .navbar-nav .dropdown:hover > .dropdown-menu > li > a, + .navbar-nav .dropdown:hover > .dropdown-menu > li > form > button, .navbar-nav .dropdown:hover > .dropdown-menu .dropdown-header, .navbar-nav .dropdown:focus-within > .dropdown-menu > li > a, + .navbar-nav .dropdown:focus-within > .dropdown-menu > li > form > button, .navbar-nav .dropdown:focus-within > .dropdown-menu .dropdown-header { padding: 5px 15px 5px 25px; } .navbar-nav .dropdown:hover > .dropdown-menu > li > a, - .navbar-nav .dropdown:focus-within > .dropdown-menu > li > a { + .navbar-nav .dropdown:hover > .dropdown-menu > li > form > button, + .navbar-nav .dropdown:focus-within > .dropdown-menu > li > a, + .navbar-nav .dropdown:focus-within > .dropdown-menu > li > form > button { line-height: 20px; } .navbar-nav .dropdown:hover > .dropdown-menu > li > a:hover, diff --git a/airflow/www/static/css/material-icons.css b/airflow/www/static/css/material-icons.css index 9ab8255f90157..55041c83b5a86 100644 --- a/airflow/www/static/css/material-icons.css +++ b/airflow/www/static/css/material-icons.css @@ -70,7 +70,9 @@ font-size: 48px; } -.dropdown-menu > li > a > .material-icons { +.dropdown-menu > li > a > .material-icons, +.dropdown-menu > li > form > button > .material-icons +{ margin-right: 6px; } diff --git a/airflow/www/templates/appbuilder/navbar_right.html b/airflow/www/templates/appbuilder/navbar_right.html index 4227d7a3cc98a..fb82a15191511 100644 --- a/airflow/www/templates/appbuilder/navbar_right.html +++ b/airflow/www/templates/appbuilder/navbar_right.html @@ -83,7 +83,12 @@
  • account_circle{{_("Your Profile")}}
    • {% endif %} -
  • exit_to_app{{_("Log Out")}}
    • +
  • +
    + + +
    +
    • {% else %} diff --git a/newsfragments/40145.significant.rst b/newsfragments/40145.significant.rst new file mode 100644 index 0000000000000..beedfc7746d43 --- /dev/null +++ b/newsfragments/40145.significant.rst @@ -0,0 +1,5 @@ +``/logout`` endpoint in FAB Auth Manager is now CSRF protected + +The ``/logout`` endpoint's method in FAB Auth Manager has been changed from ``GET`` to ``POST`` in all existing +AuthViews (``AuthDBView``, ``AuthLDAPView``, ``AuthOAuthView``, ``AuthOIDView``, ``AuthRemoteUserView``), and +now includes CSRF protection to enhance security and prevent unauthorized logouts. diff --git a/tests/providers/fab/auth_manager/api_endpoints/test_role_and_permission_endpoint.py b/tests/providers/fab/auth_manager/api_endpoints/test_role_and_permission_endpoint.py index 454fce467cffc..a91a434412d9f 100644 --- a/tests/providers/fab/auth_manager/api_endpoints/test_role_and_permission_endpoint.py +++ b/tests/providers/fab/auth_manager/api_endpoints/test_role_and_permission_endpoint.py @@ -35,6 +35,8 @@ delete_user, ) +pytestmark = pytest.mark.db_test + @pytest.fixture(scope="module") def configured_app(minimal_app_for_auth_api): diff --git a/tests/www/views/test_session.py b/tests/www/views/test_session.py index aeb9c0ffeeeb9..0ec219aaeb4b3 100644 --- a/tests/www/views/test_session.py +++ b/tests/www/views/test_session.py @@ -40,7 +40,7 @@ def test_session_inaccessible_after_logout(user_client): session_cookie = get_session_cookie(user_client) assert session_cookie is not None - resp = user_client.get("/logout/") + resp = user_client.post("/logout/") assert resp.status_code == 302 # Try to access /home with the session cookie from earlier @@ -78,7 +78,7 @@ def test_session_id_rotates(app, user_client): old_session_cookie = get_session_cookie(user_client) assert old_session_cookie is not None - resp = user_client.get("/logout/") + resp = user_client.post("/logout/") assert resp.status_code == 302 patch_path = "airflow.providers.fab.auth_manager.security_manager.override.check_password_hash"