From e6f0d8002129be90fe82fa3e3ea0a6942caba398 Mon Sep 17 00:00:00 2001
From: Patrik Dufresne
Date: Mon, 19 Dec 2022 08:18:41 -0500
Subject: [PATCH] Add CSRF verification on `/logout`
---
README.md | 3 ++-
rdiffweb/controller/tests/test_controller.py | 2 +-
.../controller/tests/test_page_admin_users.py | 2 +-
rdiffweb/controller/tests/test_page_login.py | 6 +++---
rdiffweb/controller/tests/test_page_mfa.py | 2 +-
.../controller/tests/test_secure_headers.py | 2 +-
rdiffweb/templates/layout.html | 17 ++++++++++-------
rdiffweb/templates/mfa.html | 10 ++++++----
rdiffweb/test.py | 2 +-
rdiffweb/tools/auth_form.py | 2 ++
10 files changed, 28 insertions(+), 20 deletions(-)
diff --git a/README.md b/README.md
index e1a8a35f..8fdc45e3 100644
--- a/README.md
+++ b/README.md
@@ -111,7 +111,8 @@ Professional support for Rdiffweb is available by contacting [IKUS Soft](https:/
## Next Release - 2.5.4
* Discard `X-Forwarded-Host` headers credit to [Anishka Shukla](https://github.com/anishkashukla)
-* Create proper symbolic link of chartkick.js on Ubuntu Jammy to fix loading of Charts in web interface
+* Create proper symbolic link of `chartkick.js` on Ubuntu Jammy to fix loading of Charts in web interface
+* Add CSRF verification on `/logout` credits to [reza.duty](https://rezaduty.me)
## 2.5.3 (2022-12-05)
diff --git a/rdiffweb/controller/tests/test_controller.py b/rdiffweb/controller/tests/test_controller.py
index 51e84b85..8badc037 100644
--- a/rdiffweb/controller/tests/test_controller.py
+++ b/rdiffweb/controller/tests/test_controller.py
@@ -77,7 +77,7 @@ def test_static_files(self, path):
"""
Check if the theme is properly configure.
"""
- self.getPage('/logout')
+ self.getPage('/logout', method="POST")
self.getPage(path)
self.assertStatus(200)
# Test with invalid method.
diff --git a/rdiffweb/controller/tests/test_page_admin_users.py b/rdiffweb/controller/tests/test_page_admin_users.py
index 55e8a1c3..f3bbdbfc 100644
--- a/rdiffweb/controller/tests/test_page_admin_users.py
+++ b/rdiffweb/controller/tests/test_page_admin_users.py
@@ -325,7 +325,7 @@ def test_delete_user_admin(self):
"""
# Create another admin user
self._add_user('admin2', '', 'pr3j5Dwi', '', UserObject.ADMIN_ROLE)
- self.getPage("/logout")
+ self.getPage("/logout", method="POST")
self.assertStatus(303)
self.assertHeaderItemValue('Location', self.baseurl + '/')
self._login('admin2', 'pr3j5Dwi')
diff --git a/rdiffweb/controller/tests/test_page_login.py b/rdiffweb/controller/tests/test_page_login.py
index 5aa89fc0..44ad9656 100644
--- a/rdiffweb/controller/tests/test_page_login.py
+++ b/rdiffweb/controller/tests/test_page_login.py
@@ -183,7 +183,7 @@ def test_login_twice(self):
def test_login_persistent(self):
# Given a user authenticated with persistent
- self.getPage('/logout/')
+ self.getPage('/logout', method="POST")
self.assertStatus(303)
self.getPage(
'/login/', method='POST', body={'login': self.USERNAME, 'password': self.PASSWORD, 'persistent': '1'}
@@ -312,7 +312,7 @@ class LogoutPageTest(rdiffweb.test.WebCase):
def test_getpage_without_login(self):
# Given an unauthenticated user
# When Accessing logout page directly
- self.getPage('/logout')
+ self.getPage('/logout', method="POST")
# Then user is redirect to root '/'
self.assertStatus('303 See Other')
self.assertHeaderItemValue('Location', self.baseurl + '/')
@@ -332,7 +332,7 @@ def test_getpage_with_login(self):
self.getPage("/prefs/general")
self.assertStatus('200 OK')
# When logout
- self.getPage('/logout')
+ self.getPage('/logout', method="POST")
# Then a new session id is generated
self.assertNotEqual(prev_session_id, self.session_id)
# Then user is redirected to root page
diff --git a/rdiffweb/controller/tests/test_page_mfa.py b/rdiffweb/controller/tests/test_page_mfa.py
index a84c8ab1..507d3d3e 100644
--- a/rdiffweb/controller/tests/test_page_mfa.py
+++ b/rdiffweb/controller/tests/test_page_mfa.py
@@ -52,7 +52,7 @@ def setUp(self):
def test_get_without_login(self):
# Given an unauthenticated user
- self.getPage("/logout")
+ self.getPage("/logout", method="POST")
self.assertStatus(303)
# When requesting /mfa/
self.getPage("/mfa/")
diff --git a/rdiffweb/controller/tests/test_secure_headers.py b/rdiffweb/controller/tests/test_secure_headers.py
index 37538337..9e81f79a 100644
--- a/rdiffweb/controller/tests/test_secure_headers.py
+++ b/rdiffweb/controller/tests/test_secure_headers.py
@@ -58,7 +58,7 @@ def test_cookie_with_https(self):
('/invalid', 404),
('/browse/invalid', 404),
('/login', 301),
- ('/logout', 303),
+ ('/logout', 405),
]
)
def test_cookie_with_https_http_error(self, url, expected_error_code):
diff --git a/rdiffweb/templates/layout.html b/rdiffweb/templates/layout.html
index 89b2445a..aa578383 100644
--- a/rdiffweb/templates/layout.html
+++ b/rdiffweb/templates/layout.html
@@ -85,13 +85,16 @@
{{ username }}
-
+
diff --git a/rdiffweb/templates/mfa.html b/rdiffweb/templates/mfa.html
index 0a6ff730..fbbae815 100644
--- a/rdiffweb/templates/mfa.html
+++ b/rdiffweb/templates/mfa.html
@@ -11,10 +11,12 @@ {% trans %}Login Verification{% endtrans %}
{{ form }}
-
- {% trans %}Login with a different account{% endtrans %}
-
+ {# Logout button #}
+
diff --git a/rdiffweb/test.py b/rdiffweb/test.py
index 05e64381..c016c196 100644
--- a/rdiffweb/test.py
+++ b/rdiffweb/test.py
@@ -176,7 +176,7 @@ def getJson(self, *args, **kwargs):
return json.loads(self.body.decode('utf8'))
def _login(self, username=USERNAME, password=PASSWORD):
- self.getPage("/logout")
+ self.getPage("/logout", method="POST")
self.getPage("/login/", method='POST', body={'login': username, 'password': password})
self.assertStatus('303 See Other')
diff --git a/rdiffweb/tools/auth_form.py b/rdiffweb/tools/auth_form.py
index 7a2442c9..62047f44 100644
--- a/rdiffweb/tools/auth_form.py
+++ b/rdiffweb/tools/auth_form.py
@@ -104,6 +104,8 @@ def run(self, login_url='/login/', logout_url='/logout', persistent_timeout=4320
# Clear session when browsing /logout
if request.path_info == logout_url or request.path_info.startswith(logout_url):
+ if request.method != 'POST':
+ raise cherrypy.HTTPError(405)
self.logout()
raise cherrypy.HTTPRedirect('/')