Fix expiration dates and expire csrf tokens properly

In client.py: add explicit expiration of csrf tokens to
handle_csrf. There is a clean_up() that runs on every client
connection before handle)csrf is invoked, but it only cleans every
hour. With short lived tokens this is insufficient. Also remove
debugging.

In templating.py fix values for seconds/week and minutes per week. The
original values were shifted/transposed and an order of magnitude off.

In test_templating.py again fix seconds/week constant.

Author: rouilj

roundup/cgi/client.py

@@ -1086,18 +1086,25 @@ def handle_csrf(self, xmlrpc=False):
                     logger.warning(self._("required csrf field missing for user%s"), user)
                 return True
 
+        # Expire old csrf tokens now so we don't use them.  These will
+        # be committed after the otks.destroy below.  Note that the
+        # self.clean_up run as part of determine_user() will run only
+        # once an hour. If we have short lived (e.g. 5 minute) keys
+        # they will live too long if we depend on clean_up. So we do
+        # our own.
+        otks.clean()
+
         key=self.form['@csrf'].value
         uid = otks.get(key, 'uid', default=None)
         sid = otks.get(key, 'sid', default=None)
-        if __debug__:
-            ts = otks.get(key, '__timestamp', default=None)
-            print("Found key %s for user%s sess: %s, ts %s, time %s"%(key, uid, sid, ts, time.time()))
-        current_session = self.session_api._sid
 
-        # The key has been used or compromised. Delete it to prevent replay.
+        # The key has been used or compromised.
+        # Delete it to prevent replay.
         otks.destroy(key)
         self.db.commit()
 
+        current_session = self.session_api._sid
+
         '''
         # I think now that LogoutAction redirects to
         # self.base ([tracker] web parameter in config.ini),

roundup/cgi/templating.py

@@ -97,12 +97,13 @@ def anti_csrf_nonce(self, client, lifetime=None):
     # That's the cleanup period hardcoded in otk.clean().
     # If a user wants a 10 minute lifetime calculate
     # 10 minutes newer than 1 week ago.
-    #   lifetime - 10800 (number of minutes in a week)
+    #   lifetime - 10080 (number of minutes in a week)
     # convert to seconds and add (possible negative number)
-    # from time.time().
+    # to current time (time.time()).
+    ts = time.time() + ((lifetime - 10080) * 60)
     otks.set(key, uid=client.db.getuid(),
              sid=client.session_api._sid,
-             __timestamp=time.time() + ((lifetime - 10800) * 60) )
+             __timestamp=ts )
     client.db.commit()
     return key
 

test/test_templating.py

@@ -143,8 +143,13 @@ def test_anti_csrf_nonce(self):
         '''
 
         # the value below is number of seconds in a week.
-        week_seconds = 648000 
+        week_seconds = 604800
+
+        otks=self.client.db.getOTKManager()
+
         for test in [ 'module', 'template', 'default_time' ]:
+            print "Testing:", test
+            
             if test == 'module':
                 # test the module function
                 nonce1 = anti_csrf_nonce(self, self.client, lifetime=1)
@@ -162,7 +167,6 @@ def test_anti_csrf_nonce(self):
                 greater_than = week_seconds - 10 * 60
 
             self.assertEqual(len(nonce1), 64)
-            otks=self.client.db.getOTKManager()
 
             uid = otks.get(nonce1, 'uid', default=None)
             sid = otks.get(nonce1, 'sid', default=None)
@@ -171,17 +175,19 @@ def test_anti_csrf_nonce(self):
             self.assertEqual(uid, 10) 
             self.assertEqual(sid, self.client.session_api._sid)
 
-            ts = time.time()
+            now = time.time()
+
+            print "now, timestamp, greater, difference", \
+                     now, timestamp, greater_than, now - timestamp
+
 
             # lower bound of the difference is above. Upper bound
             # of difference is run time between time.time() in
             # the call to anti_csrf_nonce and the time.time() call
             # that assigns ts above. I declare that difference
             # to be less than 1 second for this to pass.
             self.assertEqual(True,
-                       greater_than < ts - timestamp < (greater_than + 1) )
-
-            print "completed", test
+                       greater_than <= now - timestamp < (greater_than + 1) )
 
     def test_string_url_quote(self):
         ''' test that urlquote quotes the string '''