Add CSRF verification on /logout

ikus060 · Dec 19, 2022 · e6f0d80 · e6f0d80
1 parent 4392987
commit e6f0d80
Show file tree

Hide file tree

Showing 10 changed files with 28 additions and 20 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -85,13 +85,16 @@
                   <i aria-hidden="true" class="fa fa-user"></i>
                   {{ username }}
                 </a>
-                <div aria-labelledby="navbarDropdown" class="dropdown-menu">
-                  <h6 class="dropdown-header">{% trans %}Signed in as{% endtrans %} {{ username }}</h6>
-                  <a class="dropdown-item" href="{{ url_for('prefs', 'general') }}">{% trans %}Edit profile{% endtrans %}</a>
-                  <a class="dropdown-item" href="{{ url_for('prefs', 'notification') }}">{% trans %}Notifications{% endtrans %}</a>
-                  <div class="dropdown-divider"></div>
-                  <a class="dropdown-item" href="{{ url_for('logout') }}">Logout</a>
-                </div>
+                <form action="{{ url_for('logout') }}" method="post">
+                  <div aria-labelledby="navbarDropdown" class="dropdown-menu">
+                    <h6 class="dropdown-header">{% trans %}Signed in as{% endtrans %} {{ username }}</h6>
+                    <a class="dropdown-item" href="{{ url_for('prefs', 'general') }}">{% trans %}Edit profile{% endtrans %}</a>
+                    <a class="dropdown-item" href="{{ url_for('prefs', 'notification') }}">{% trans %}Notifications{% endtrans %}</a>
+                    <div class="dropdown-divider"></div>
+                    {# Logout button #}
+                    <button type="submit" class="dropdown-item">{% trans %}Logout{% endtrans %}</button>
+                  </div>
+                </form>
               </li>
             </ul>
           </div>

diff --git a/rdiffweb/templates/mfa.html b/rdiffweb/templates/mfa.html
@@ -11,10 +11,12 @@ <h2>{% trans %}Login Verification{% endtrans %}</h2>
           </p>
           {{ form }}
         </form>
-        <a class="btn-link btn-sm btn-block text-center"
-           href="{{ url_for('logout')}}">
-          {% trans %}Login with a different account{% endtrans %}
-        </a>
+        {# Logout button #}
+        <form action="{{ url_for('logout') }}" method="post">
+          <button type="submit" class="btn btn-link btn-sm btn-block text-center">
+            {% trans %}Login with a different account{% endtrans %}
+          </button>
+        </form>
       </div>
     </div>
   </div>

diff --git 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
@@ -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('/')