security: apply CSRF check to all non-GET requests

The automatic CSRF protection was broken for POST requests with no
request payload parameters (but possibly containing request URI
parameters); a security hole was narrowly avoided because the code
base quite consistently checks the request method in the same way,
and because of browser protection against PUT/DELETE CSRF attacks.

Since explicit is better than implicit, the better way of checking
the HTTP request method is to simply check request.method, instead
of checking if request.POST is non-empty, which is subtly different
(it doesn't catch POST requests if all parameters are in the query
string) and non-obvious (because it also applies to PUT requests).

The commit also fixes some tests which relied on the CSRF protection
being broken. It does not fix all the controllers that still does
the misleading request.POST check, but since the CSRF check has now
been tightened, those are no longer a potential security issue.

kallithea/lib/auth.py

@@ -760,8 +760,13 b' class LoginRequired(object):'
                 log.warning('API access to %s is not allowed', loc)
                 return abort(403)
 
-        # CSRF protection - POSTs with session auth must contain correct token
-        if request.POST and user.is_authenticated:
+        # CSRF protection: Whenever a request has ambient authority (whether
+        # through a session cookie or its origin IP address), it must include
+        # the correct token, unless the HTTP method is GET or HEAD (and thus
+        # guaranteed to be side effect free.
+        # Note that the 'is_authenticated' flag is True for anonymous users too,
+        # but not when the user is authenticated by API key.
+        if user.is_authenticated and request.method not in ['GET', 'HEAD']:
             token = request.POST.get(secure_form.token_key)
             if not token or token != secure_form.authentication_token():
                 log.error('CSRF check failed')

kallithea/tests/functional/test_admin_defaults.py

@@ -16,7 +16,8 b' class TestDefaultsController(TestControl'
         response = self.app.get(url('formatted_defaults', format='xml'))
 
     def test_create(self):
-        response = self.app.post(url('defaults'))
+        response = self.app.post(url('defaults'),
+            {'_authentication_token': self.authentication_token()})
 
     def test_new(self):
         response = self.app.get(url('new_default'))
@@ -62,7 +63,8 b' class TestDefaultsController(TestControl'
         response = self.app.post(url('default', id=1), params=dict(_method='put', _authentication_token=self.authentication_token()))
 
     def test_delete(self):
-        response = self.app.delete(url('default', id=1))
+        # Not possible due to CSRF protection.
+        response = self.app.delete(url('default', id=1), status=403)
 
     def test_delete_browser_fakeout(self):
         response = self.app.post(url('default', id=1), params=dict(_method='delete', _authentication_token=self.authentication_token()))

kallithea/tests/functional/test_admin_gists.py

@@ -136,19 +136,20 b' class TestGistsController(TestController'
     def test_delete(self):
         self.log_user()
         gist = _create_gist('delete-me')
-        response = self.app.delete(url('gist', gist_id=gist.gist_id))
-        self.checkSessionFlash(response, 'Deleted gist %s' % gist.gist_id)
+        response = self.app.post(url('gist', gist_id=gist.gist_id),
+            params={'_method': 'delete', '_authentication_token': self.authentication_token()})
 
     def test_delete_normal_user_his_gist(self):
         self.log_user(TEST_USER_REGULAR_LOGIN, TEST_USER_REGULAR_PASS)
         gist = _create_gist('delete-me', owner=TEST_USER_REGULAR_LOGIN)
-        response = self.app.delete(url('gist', gist_id=gist.gist_id))
-        self.checkSessionFlash(response, 'Deleted gist %s' % gist.gist_id)
+        response = self.app.post(url('gist', gist_id=gist.gist_id),
+            params={'_method': 'delete', '_authentication_token': self.authentication_token()})
 
     def test_delete_normal_user_not_his_own_gist(self):
         self.log_user(TEST_USER_REGULAR_LOGIN, TEST_USER_REGULAR_PASS)
         gist = _create_gist('delete-me')
-        response = self.app.delete(url('gist', gist_id=gist.gist_id), status=403)
+        response = self.app.post(url('gist', gist_id=gist.gist_id), status=403,
+            params={'_method': 'delete', '_authentication_token': self.authentication_token()})
 
     def test_show(self):
         gist = _create_gist('gist-show-me')

kallithea/tests/functional/test_admin_notifications.py

@@ -56,9 +56,9 b' class TestNotificationsController(TestCo'
         self.assertEqual(get_notif(u2.notifications), [notification])
         cur_usr_id = cur_user.user_id
 
-        response = self.app.delete(url('notification',
-                                       notification_id=
-                                       notification.notification_id))
+        response = self.app.post(
+            url('notification', notification_id=notification.notification_id),
+            params={'_method': 'delete', '_authentication_token': self.authentication_token()})
         self.assertEqual(response.body, 'ok')
 
         cur_user = User.get(cur_usr_id)

kallithea/tests/functional/test_admin_repos.py

@@ -398,7 +398,8 b' class _BaseTest(object):'
         except vcs.exceptions.VCSError:
             self.fail('no repo %s in filesystem' % repo_name)
 
-        response = self.app.delete(url('delete_repo', repo_name=repo_name))
+        response = self.app.post(url('delete_repo', repo_name=repo_name),
+            params={'_method': 'delete', '_authentication_token': self.authentication_token()})
 
         self.checkSessionFlash(response, 'Deleted repository %s' % (repo_name))
 
@@ -450,7 +451,8 b' class _BaseTest(object):'
         except vcs.exceptions.VCSError:
             self.fail('no repo %s in filesystem' % repo_name)
 
-        response = self.app.delete(url('delete_repo', repo_name=repo_name))
+        response = self.app.post(url('delete_repo', repo_name=repo_name),
+            params={'_method': 'delete', '_authentication_token': self.authentication_token()})
         self.checkSessionFlash(response, 'Deleted repository %s' % (repo_name_unicode))
         response.follow()
 

kallithea/tests/functional/test_admin_user_groups.py

@@ -32,7 +32,7 b' class TestAdminUsersGroupsController(Tes'
         response = self.app.get(url('new_users_group'))
 
     def test_update(self):
-        response = self.app.put(url('users_group', id=1))
+        response = self.app.put(url('users_group', id=1), status=403)
 
     def test_update_browser_fakeout(self):
         response = self.app.post(url('users_group', id=1),
@@ -54,7 +54,8 b' class TestAdminUsersGroupsController(Tes'
         gr = Session().query(UserGroup)\
             .filter(UserGroup.users_group_name == users_group_name).one()
 
-        response = self.app.delete(url('users_group', id=gr.users_group_id))
+        response = self.app.post(url('users_group', id=gr.users_group_id),
+            params={'_method': 'delete', '_authentication_token': self.authentication_token()})
 
         gr = Session().query(UserGroup)\
             .filter(UserGroup.users_group_name == users_group_name).scalar()
@@ -97,7 +98,8 b' class TestAdminUsersGroupsController(Tes'
 
         ## DISABLE REPO CREATE ON A GROUP
         response = self.app.put(
-            url('edit_user_group_default_perms', id=ug.users_group_id), {})
+            url('edit_user_group_default_perms', id=ug.users_group_id),
+            params={'_authentication_token': self.authentication_token()})
 
         response.follow()
         ug = UserGroup.get_by_group_name(users_group_name)
@@ -119,7 +121,8 b' class TestAdminUsersGroupsController(Tes'
         # DELETE !
         ug = UserGroup.get_by_group_name(users_group_name)
         ugid = ug.users_group_id
-        response = self.app.delete(url('users_group', id=ug.users_group_id))
+        response = self.app.post(url('users_group', id=ug.users_group_id),
+            params={'_method': 'delete', '_authentication_token': self.authentication_token()})
         response = response.follow()
         gr = Session().query(UserGroup)\
             .filter(UserGroup.users_group_name == users_group_name).scalar()
@@ -167,8 +170,8 b' class TestAdminUsersGroupsController(Tes'
                     [ug.users_group_id, p3.permission_id]]))
 
         ## DISABLE REPO CREATE ON A GROUP
-        response = self.app.put(
-            url('edit_user_group_default_perms', id=ug.users_group_id), {})
+        response = self.app.put(url('edit_user_group_default_perms', id=ug.users_group_id),
+            params={'_authentication_token': self.authentication_token()})
 
         response.follow()
         ug = UserGroup.get_by_group_name(users_group_name)
@@ -189,7 +192,8 b' class TestAdminUsersGroupsController(Tes'
         # DELETE !
         ug = UserGroup.get_by_group_name(users_group_name)
         ugid = ug.users_group_id
-        response = self.app.delete(url('users_group', id=ug.users_group_id))
+        response = self.app.post(url('users_group', id=ug.users_group_id),
+            params={'_method': 'delete', '_authentication_token': self.authentication_token()})
         response = response.follow()
         gr = Session().query(UserGroup)\
                            .filter(UserGroup.users_group_name ==

kallithea/tests/functional/test_admin_users.py

@@ -167,7 +167,8 b' class TestAdminUsersController(TestContr'
 
         new_user = Session().query(User)\
             .filter(User.username == username).one()
-        response = self.app.delete(url('user', id=new_user.user_id))
+        response = self.app.post(url('user', id=new_user.user_id),
+            params={'_method': 'delete', '_authentication_token': self.authentication_token()})
 
         self.checkSessionFlash(response, 'Successfully deleted user')
 
@@ -181,16 +182,19 b' class TestAdminUsersController(TestContr'
 
         new_user = Session().query(User)\
             .filter(User.username == username).one()
-        response = self.app.delete(url('user', id=new_user.user_id))
+        response = self.app.post(url('user', id=new_user.user_id),
+            params={'_method': 'delete', '_authentication_token': self.authentication_token()})
         self.checkSessionFlash(response, 'User "%s" still '
                                'owns 1 repositories and cannot be removed. '
                                'Switch owners or remove those repositories: '
                                '%s' % (username, reponame))
 
-        response = self.app.delete(url('delete_repo', repo_name=reponame))
+        response = self.app.post(url('delete_repo', repo_name=reponame),
+            params={'_method': 'delete', '_authentication_token': self.authentication_token()})
         self.checkSessionFlash(response, 'Deleted repository %s' % reponame)
 
-        response = self.app.delete(url('user', id=new_user.user_id))
+        response = self.app.post(url('user', id=new_user.user_id),
+            params={'_method': 'delete', '_authentication_token': self.authentication_token()})
         self.checkSessionFlash(response, 'Successfully deleted user')
 
     def test_delete_repo_group_err(self):
@@ -203,7 +207,8 b' class TestAdminUsersController(TestContr'
 
         new_user = Session().query(User)\
             .filter(User.username == username).one()
-        response = self.app.delete(url('user', id=new_user.user_id))
+        response = self.app.post(url('user', id=new_user.user_id),
+            params={'_method': 'delete', '_authentication_token': self.authentication_token()})
         self.checkSessionFlash(response, 'User "%s" still '
                                'owns 1 repository groups and cannot be removed. '
                                'Switch owners or remove those repository groups: '
@@ -213,10 +218,12 b' class TestAdminUsersController(TestContr'
         # rg = RepoGroup.get_by_group_name(group_name=groupname)
         # response = self.app.get(url('repos_groups', id=rg.group_id))
 
-        response = self.app.delete(url('delete_repo_group', group_name=groupname))
+        response = self.app.post(url('delete_repo_group', group_name=groupname),
+            params={'_method': 'delete', '_authentication_token': self.authentication_token()})
         self.checkSessionFlash(response, 'Removed repository group %s' % groupname)
 
-        response = self.app.delete(url('user', id=new_user.user_id))
+        response = self.app.post(url('user', id=new_user.user_id),
+            params={'_method': 'delete', '_authentication_token': self.authentication_token()})
         self.checkSessionFlash(response, 'Successfully deleted user')
 
     def test_delete_user_group_err(self):
@@ -229,7 +236,8 b' class TestAdminUsersController(TestContr'
 
         new_user = Session().query(User)\
             .filter(User.username == username).one()
-        response = self.app.delete(url('user', id=new_user.user_id))
+        response = self.app.post(url('user', id=new_user.user_id),
+            params={'_method': 'delete', '_authentication_token': self.authentication_token()})
         self.checkSessionFlash(response, 'User "%s" still '
                                'owns 1 user groups and cannot be removed. '
                                'Switch owners or remove those user groups: '
@@ -241,7 +249,8 b' class TestAdminUsersController(TestContr'
 
         fixture.destroy_user_group(ug.users_group_id)
 
-        response = self.app.delete(url('user', id=new_user.user_id))
+        response = self.app.post(url('user', id=new_user.user_id),
+            params={'_method': 'delete', '_authentication_token': self.authentication_token()})
         self.checkSessionFlash(response, 'Successfully deleted user')
 
     def test_show(self):

kallithea/tests/functional/test_changeset_comments.py

@@ -132,10 +132,11 b' class TestChangeSetCommentsController(Te'
         self.assertEqual(len(comments), 1)
         comment_id = comments[0].comment_id
 
-        self.app.delete(url(controller='changeset',
+        self.app.post(url(controller='changeset',
                                     action='delete_comment',
                                     repo_name=HG_REPO,
-                                    comment_id=comment_id))
+                                    comment_id=comment_id),
+            params={'_method': 'delete', '_authentication_token': self.authentication_token()})
 
         comments = ChangesetComment.query().all()
         self.assertEqual(len(comments), 0)

kallithea/tests/functional/test_forks.py

@@ -96,7 +96,8 b' class _BaseTestCase(object):'
         )
 
         # remove this fork
-        response = self.app.delete(url('delete_repo', repo_name=fork_name))
+        response = self.app.post(url('delete_repo', repo_name=fork_name),
+            params={'_method': 'delete', '_authentication_token': self.authentication_token()})
 
     def test_fork_create_into_group(self):
         self.log_user()

kallithea/tests/functional/test_my_account.py

@@ -57,7 +57,8 b' class TestMyAccountController(TestContro'
         self.log_user()
         response = self.app.get(url('my_account_emails'))
         response.mustcontain('No additional emails specified')
-        response = self.app.post(url('my_account_emails'),)
+        response = self.app.post(url('my_account_emails'),
+            {'_authentication_token': self.authentication_token()})
         self.checkSessionFlash(response, 'Please enter an email address')
 
     def test_my_account_my_emails_add_remove(self):