set_cookie_expires = False breaks HTTP headers with CookieWriter

[posita]

I can't quite track down why this is happening, but it seems that when set_cookie_expires = False and writer = 'cookie', the cookie being set will "interrupt" the HTTP headers (so that subsequent headers are treated as if they are part of the response body).

Here's some test code that will toggle set_cookie_expires every other refresh (so you can see the difference by hitting reload):

import appengine_utilities.sessions
import google.appengine.ext.webapp as webapp
class CookieTest(webapp.RequestHandler):
    TOGGLE = False
    def get(self):
        CookieTest.TOGGLE = not CookieTest.TOGGLE
        session =  appengine_utilities.sessions.Session(
                cookie_name = 'test',
                integrate_flash = False,
                session_expire_time = 180 * 24 * 60 * 60, # 180 days                                                                    
                set_cookie_expires = CookieTest.TOGGLE,
                writer = 'cookie',
            )
        self.response.headers['Content-Type'] = 'text/plain'
        self.response.out.write("You've been Kelly Ripa'd!\r\n\r\n(set_cookie_expires = %s)" % CookieTest.TOGGLE)

Note this only happens if it is the first session to be written. In other words, this seems to work okay:

import appengine_utilities.sessions
import google.appengine.ext.webapp as webapp
class ThisCookieTestWorks(webapp.RequestHandler):
    def get(self):
        session_kwargs = {
                'integrate_flash': False,
                'session_expire_time': 180 * 24 * 60 * 60, # 180 days                                                                    
                'writer': 'cookie',
        }
        session_kwargs['cookie_name']: 'test1',
        session_kwargs['set_cookie_expires']: True,
        session =  appengine_utilities.sessions.Session(**session_kwargs)
        session_kwargs['cookie_name']: 'test2',
        session_kwargs['set_cookie_expires']: False,
        session =  appengine_utilities.sessions.Session(**session_kwargs)
        self.response.headers['Content-Type'] = 'text/plain'
        self.response.out.write("You've been Kelly Ripa'd!")

[posita]

I just thought of something. What if instead of using 'print' statements to output the cookies, session accessed a Headers object passed in to its constructor?

I may have a patch soon....

[posita]

This seems to work:

diff -Naru appengine_utilities.orig/sessions.py appengine_utilities/sessions.py
--- appengine_utilities.orig/sessions.py    2009-09-27 20:20:26.000000000 -0700
+++ appengine_utilities/sessions.py 2010-03-16 14:37:02.000000000 -0700
@@ -34,6 +34,7 @@
 import hashlib
 import Cookie
 import pickle
+import warnings
 import __main__
 from time import strftime

@@ -374,7 +375,7 @@
             del(session.cookie_vals[keyname])
             session.output_cookie["%s_data" % (session.cookie_name)] = \
                 simplejson.dumps(session.cookie_vals)
-            print session.output_cookie.output()
+            session.output_cookie_headers()

         sessdata = session._get(keyname=keyname)
         if sessdata is None:
@@ -417,7 +418,7 @@
         # so let it raise exceptions
         session.output_cookie["%s_data" % (session.cookie_name)] = \
             simplejson.dumps(session.cookie_vals)
-        print session.output_cookie.output()
+        session.output_cookie_headers()
         return True

 class Session(object):
@@ -505,7 +506,8 @@
             set_cookie_expires=settings.session["SET_COOKIE_EXPIRES"],
             session_token_ttl=settings.session["SESSION_TOKEN_TTL"],
             last_activity_update=settings.session["UPDATE_LAST_ACTIVITY"],
-            writer=settings.session["WRITER"]):
+            writer=settings.session["WRITER"],
+            wsgiref_headers=None):
         """
         Initializer

@@ -524,6 +526,9 @@
               it saves even if the browser is closed.
           session_token_ttl: Number of sessions a session token is valid
               for before it should be regenerated.
+          wsgiref_headers: the wsgiref.headers.Headers object to which to
+              write any Set-Cookie headers (if None, headers will be
+              written to stdout).
         """

         self.cookie_path = cookie_path
@@ -536,9 +541,13 @@
         self.session_token_ttl = session_token_ttl
         self.last_activity_update = last_activity_update
         self.writer = writer
+        self.wsgiref_headers = wsgiref_headers
+
+        if self.wsgiref_headers is None:
+            warnings.warn('using Session without wsgiref_headers is deprecated (see <http://github.com/joerussbowman/gaeutilities/issues/#issue/12>)', DeprecationWarning, stacklevel = 2)

         # make sure the page is not cached in the browser
-        print self.no_cache_headers()
+        self.output_no_cache_headers()
         # Check the cookie and, if necessary, create a new one.
         self.cache = {}
         string_cookie = os.environ.get(u"HTTP_COOKIE", u"")
@@ -552,9 +561,9 @@
                 # values available for all gets immediately.
             for k in self.cookie_vals:
                 self.cache[k] = self.cookie_vals[k]
-                # sync the input cookie with the output cookie
-                self.output_cookie["%s_data" % (self.cookie_name)] = \
-                    self.cookie["%s_data" % (self.cookie_name)]
+            # sync the input cookie with the output cookie
+            self.output_cookie["%s_data" % (self.cookie_name)] = \
+                simplejson.dumps(self.cookie_vals) #self.cookie["%s_data" % (self.cookie_name)]
         except:
             self.cookie_vals = {}

@@ -629,7 +638,7 @@
                 self.output_cookie["%s_data" % (cookie_name)] = u""
             self.output_cookie["%s_data" % (cookie_name)]["expires"] = \
                 self.session_expire_time
-        print self.output_cookie.output()
+        self.output_cookie_headers()

         # fire up a Flash object if integration is enabled
         if self.integrate_flash:
@@ -730,7 +739,7 @@
         self.cache = {}
         self.output_cookie["%s_data" % (self.cookie_name)] = \
             simplejson.dumps(self.cookie_vals)
-        print self.output_cookie.output()
+        self.output_cookie_headers()
         # if the event class has been loaded, fire off the sessionDelete event
         if u"AEU_Events" in __main__.__dict__:
             __main__.AEU_Events.fire_event(u"sessionDelete")
@@ -814,19 +823,40 @@
         self.__init__()
         return True

-    def no_cache_headers(self):
+    def output_no_cache_headers(self):
         """
         Generates headers to avoid any page caching in the browser.
         Useful for highly dynamic sites.

         Returns a unicode string of headers.
         """
-        return u"".join([u"Expires: Tue, 03 Jul 2001 06:00:00 GMT",
-            strftime("Last-Modified: %a, %d %b %y %H:%M:%S %Z").decode("utf-8"),
-            u"Cache-Control: no-store, no-cache, must-revalidate, max-age=0",
-            u"Cache-Control: post-check=0, pre-check=0",
-            u"Pragma: no-cache",
-        ])
+        if self.wsgiref_headers is not None:
+            self.wsgiref_headers.add_header('Expires', 'Tue, 03 Jul 2001 06:00:00 GMT')
+            self.wsgiref_headers.add_header('Last-Modified', strftime('%a, %d %b %y %H:%M:%S %Z'))
+            self.wsgiref_headers.add_header('Cache-Control', 'no-store, no-cache, must-revalidate, max-age=0')
+            self.wsgiref_headers.add_header('Cache-Control', 'post-check=0, pre-check=0')
+            self.wsgiref_headers.add_header('Pragma', 'no-cache')
+        else:
+            print u"".join([u"Expires: Tue, 03 Jul 2001 06:00:00 GMT",
+                strftime("Last-Modified: %a, %d %b %y %H:%M:%S %Z").decode("utf-8"),
+                u"Cache-Control: no-store, no-cache, must-revalidate, max-age=0",
+                u"Cache-Control: post-check=0, pre-check=0",
+                u"Pragma: no-cache",
+            ])
+
+    def output_cookie_headers(self, cookie = None):
+        """
+        Adds the cookie to the wsgiref headers (if present), otherwise,
+        prints Set-Cookie headers to stdout.
+        """
+        if cookie is None:
+            cookie = self.output_cookie
+
+        if self.wsgiref_headers is not None:
+            for morsel in cookie.itervalues():
+                self.wsgiref_headers.add_header('Set-Cookie', morsel.OutputString())
+        else:
+            print cookie.output()

     def clear(self):
         """
@@ -846,7 +876,7 @@
         self.cookie_vals = {}
         self.output_cookie["%s_data" %s (self.cookie_name)] = \
             simplejson.dumps(self.cookie_vals)
-        print self.output_cookie.output()
+        self.output_cookie_headers()
         return True

     def has_key(self, keyname):
@@ -971,7 +1001,7 @@
                     output_cookie = Cookie.SimpleCookie()
                     output_cookie[cookie_name] = cookie[cookie_name]
                     output_cookie[cookie_name][u"expires"] = 0
-                    print output_cookie.output()
+                    self.output_cookie_headers(output_cookie)
         return False

     def get_ds_entity(self):
@@ -1074,7 +1104,7 @@
             bad_key = False
             self.output_cookie["%s_data" % (self.cookie_name)] = \
                 simplejson.dumps(self.cookie_vals)
-            print self.output_cookie.output()
+            self.output_cookie_headers()
         if bad_key:
             raise KeyError(unicode(keyname))
         if keyname in self.cache:

Now you can do something like this:

import google.appengine.ext.webapp as webapp
class CookieTest(webapp.RequestHandler):
    TOGGLE = False
    def get(self):
        CookieTest.TOGGLE = not CookieTest.TOGGLE
        session =  appengine_utilities.sessions.Session(
                cookie_name = 'test',
                integrate_flash = False,
                session_expire_time = 180 * 24 * 60 * 60, # 180 days                                                                    
                set_cookie_expires = CookieTest.TOGGLE,
                writer = 'cookie',
                wsgiref_headers = self.request.headers,
            )
        self.response.headers['Content-Type'] = 'text/plain'
        self.response.out.write("You've been Kelly Ripa'd!\r\n\r\n(set_cookie_expires = %s)" % CookieTest.TOGGLE)

I have not yet done the same thing with flash.py though....

[posita]

I just noticed something. In sessions.py there's this:

print u"".join([u"Expires: Tue, 03 Jul 2001 06:00:00 GMT",
    ...

I might be missing something, but shouldn't that be:

print u"\r\n".join([u"Expires: Tue, 03 Jul 2001 06:00:00 GMT",
    ...

? I definitely do not claim to know all about cookies or HTTP headers, so I might be completely off my rocker. (The same thing appears in flash.py.) I don't think this has any effect or bearing on the above bug, I just noticed it while I was hacking away at sessions.py to create the patch.

[joerussbowman]

not sure if you're still following this issue... I'd love to get a pull request with this patch against the current version. I'm rewriting a lot of how session works with the datastore right now, so you may want to wait until that's done.

[joerussbowman]

oh that's right, you have write access to this repo, duh... as I said working on rewriting the datastore access, but that should be done in a few days, hopefully today if I can get it all in and tested.

[dound]

I'm having trouble using the cookie-only sessions too. Even with this basic handler implementation: session = Session(writer="cookie") self.response.headers['Content-Type'] = 'text/plain' self.response.out.write('hello world')

I also tried the patch above - it stopped the headers from showing up in the body, but the set-cookie header then did not get sent at all. Maybe this problem will be easier to solve if sessions is provided as WSGI-compliant middleware instead.

Please let me know if you need more info on this issue.

[joerussbowman]

I agree with moving towards a WSGI-compliant middleware approach for session, and likely all the other utilities. It would help optimize cache write, and can get rid of the giant kludge that is event. However, this will take some time that I don't have at the moment. Maybe this fall after I finish founding my company and get into maintenance mode on my non-appengine project. Unless someone else wants to volunteer.

[dound]

Sounds good. Thanks Joe, and good luck with the company!

[joerussbowman]

just of curiosity, can you change your test to

session = Session(writer="cookie") self.response.headers['Content-Type'] = 'text/html' self.response.out.write('hello world')

Reason being, I don't think I've ever seen someone set cookies when returning plain text. I understand it should be possible, but I also understand the appengine environment likes to mess with headers.