Submitting accounts forms with a bad CSRF token leaves database transactions open

[seanh]

h.accounts.views.bad_csrf_token() (which is a BadCSRFToken exception view) opens database transactions that do not get closed.

For example add this test:

diff --git a/tests/functional/test_accounts.py b/tests/functional/test_accounts.py
index a84f368..8e7761d 100644
--- a/tests/functional/test_accounts.py
+++ b/tests/functional/test_accounts.py
@@ -46,6 +46,16 @@ class TestAccountSettings(object):

         assert res.content_type == 'text/plain'

+    def test_submit_email_form_without_csrf(self, app):
+        res = app.get('/account/settings')
+        email_form = res.forms['email']
+        email_form['email'] = '[email protected]'
+        email_form['email_confirm'] = '[email protected]'
+        email_form['password'] = 'pass'
+        res.forms['email']['csrf_token'] = None
+        for _ in range(50):
+            email_form.submit(xhr=True, status=403)
+
     def test_submit_password_form_without_xhr_returns_full_html_page(self,
                                                                      app):
         res = app.get('/account/settings')

and then py.test tests/functional/test_accounts.py -svx will freeze waiting to open a database connection.

The problem is that bad_csrf_token() calls h.session.model() which access the db. If you make this change to model() the freeze will go away:

diff --git a/h/session.py b/h/session.py
index 4ee8519..8076204 100644
--- a/h/session.py
+++ b/h/session.py
@@ -4,13 +4,10 @@
 def model(request):
     session = {}
     session['csrf'] = request.session.get_csrf_token()
-    session['userid'] = request.authenticated_userid
-    session['groups'] = _current_groups(request)
-    session['features'] = request.feature.all()
+    session['userid'] = None
+    session['groups'] = []
+    session['features'] = []
     session['preferences'] = {}
-    user = request.authenticated_user
-    if user and not user.sidebar_tutorial_dismissed:
-        session['preferences']['show_sidebar_tutorial'] = True
     return session

This isn't a solution of course but it demonstrates that accessing the db from an exception view is what causes the unclosed transactions.

The problem is that with pyramid_tm you cannot access the database from an exception view, a NewResponse callback, or a finished callback (request.add_finished_callback()). Because of the order in which pyramid_tm and exception views execute, pyramid_tm's transaction has already been committed or aborted by the time the exception view executes. So any code that ia called during an exception view and directly or indirectly uses the db will get multiple problems, including:

• DetachedInstanceErrors if the exception view tries to access certain properties of certain ORM objects. Because the session has already been committed, so ORM objects such as request.authenticated_user are now "detached" objects. If for example you access a lazy-loaded attribute of such a detached ORM object you'll get a DetachedInstanceError. This currently happens if any code (including templates) called during an exception view tries to read a feature flag, because reading feature flags accesses lazy loaded properties of request.authenticated_user (e.g. .cohorts). We don't currently have any instances of this problem that I know of in our code, but it's waiting to happen.
• Unclosed database transactions. Try to read something from the db, sqlalchemy implicitly creates a new transaction, pyramid_tm is already finished and won't close this transaction for you.

For more info about this problem in pyramid and pyramid_tm:

• move response callbacks to a tween Pylons/pyramid#2622
• add first class transaction manager support Pylons/pyramid#2606
• support using the transaction inside exception views Pylons/pyramid_tm#40
• request object is shared across retryable transaction attempts Pylons/pyramid_tm#41
• response callbacks and tweens Pylons/pyramid#1336