My testing was with dbm backends which do an automatic commit on the

otk database. My tests used a mock for the otk database because I
don't now how to get a real db into the test, so they didn't test the
rdbms code paths.

Once I committed to upstream I updated my sqlite based production
tracker and boom. Couldn't log in with csrf_enforce = required.  I was
missing calls to commit to the db which is required for rdbms
backends.

Added those and also removed redundant code where I was deleting the
otk in a few places. Once it's used and I have retrieved data from it,
I don't need it. Nuke it upstream from all the code paths that will
exit the routine so I don't have to delete in each code path.

Author: rouilj

roundup/cgi/client.py

@@ -1050,11 +1050,12 @@ def handle_csrf(self, xmlrpc=False):
         if "@csrf" not in self.form:
             if enforce == 'required':
                 self.add_error_message(self._("No csrf token found"))
+                logger.error(self._("required csrf field missing for user%s"), user)
                 return False
             else:
                 if enforce == 'logfailure':
                     # FIXME include url
-                    logger.warning(self._("csrf field missing for user%s"), user)
+                    logger.warning(self._("required csrf field missing for user%s"), user)
                 return True
 
         key=self.form['@csrf'].value
@@ -1093,28 +1094,29 @@ def handle_csrf(self, xmlrpc=False):
            self.form["@action"].value == "Login":
             if header_pass > 0:
                 otks.destroy(key)
+                self.db.commit()
                 return True
             else:
                 self.add_error_message("Reload window before logging in.")
         '''
 
+        # The key has been used or compromised. Delete it to prevent replay.
+        otks.destroy(key)
+        self.db.commit()
+
         if uid != user:
             if enforce in ('required', "yes"):
                 self.add_error_message(self._("Invalid csrf token found: %s")%key)
-                otks.destroy(key)
                 return False
             elif enforce == 'logfailure':
                     logger.warning(self._("csrf uid mismatch for user%s."), user)
-        if self.session_api._sid != sid:
+        if current_session != sid:
             if enforce in ('required', "yes"):
                 self.add_error_message(self._(
                     "Invalid csrf session found: %s")%key)
-                otks.destroy(key)
                 return False
             elif enforce == 'logfailure':
                     logger.warning(self._("csrf session mismatch for user%s."), user)
-        # Remove the key so a previous csrf key can't be replayed.
-        otks.destroy(key)
         return True
 
     def opendb(self, username):

roundup/cgi/templating.py

@@ -83,7 +83,7 @@ def anti_csrf_nonce(self, client, lifetime=None):
     # is unpredicatable (depends on number of previous connections etc.)
     key = '%s%s%s'%(random.random(),id(self),time.time())
     key = hashlib.sha256(key).hexdigest()
-        
+
     while otks.exists(key):
         key = '%s%s%s'%(random.random(),id(self),time.time())
         key = hashlib.sha256(key).hexdigest()
@@ -103,6 +103,7 @@ def anti_csrf_nonce(self, client, lifetime=None):
     otks.set(key, uid=client.db.getuid(),
              sid=client.session_api._sid,
              __timestamp=time.time() + ((lifetime - 10800) * 60) )
+    client.db.commit()
     return key
 
 ### templating