Wrong cookie is retrieved when loading trac

[aroach@…]

We have some cookie conflicts going on with our Trac installation and another site that we run. The following error is displayed when I load the Trac installation, when this other cookie is set:

Oops...

Trac detected an internal error:

Illegal key value: Password:0:JABID

Traceback (most recent call last):
  File "/var/www/localhost/cgi-bin/trac.cgi", line 25, in ?
    trac.core.cgi_start()
  File "/usr/lib/python2.4/site-packages/trac/core.py", line 533, in cgi_start
    send_pretty_error(e, None)
  File "/usr/lib/python2.4/site-packages/trac/core.py", line 473, in send_pretty_error
    req.init_request()
  File "/usr/lib/python2.4/site-packages/trac/core.py", line 371, in init_request
    self.incookie.load(os.getenv('HTTP_COOKIE'))
  File "/usr/lib/python2.4/Cookie.py", line 621, in load
    self.__ParseString(rawdata)
  File "/usr/lib/python2.4/Cookie.py", line 652, in __ParseString
    self.__set(K, rval, cval)
  File "/usr/lib/python2.4/Cookie.py", line 574, in __set
    M.set(key, real_value, coded_value)
  File "/usr/lib/python2.4/Cookie.py", line 453, in set
    raise CookieError("Illegal key value: %s" % key)
CookieError: Illegal key value: Password:0:JABID

More specifically, our support site is setting the cookie that Trac is retrieving. It's cookie is set with a jabber.com, but the trac cookie is set under trac.corp.jabber.com. I'm not sure what other information will be helpful, so I'll just stop here.

Any help would be appreciated, and good work. Trac is really helpful.

[Matthew Good]

Trac uses the standard Python libraries to parse all the cookies sent from the browser. It doesn't use the other cookie values, but it still needs to parse the headers to find the cookies it does need. That error is being throw because "Password:0:JABID" is not a legal name for a cookie since it contains ":" characters. You will need to change the name of your cookie to only include legal characters.

The ​cookie specification section 4.1 defines the cookie attribute/value pairs syntax. The attribute names must be a token which is defined in the ​HTTP/1.1 specification section 2.2 and excludes ":" as a legal character:

          token          = 1*<any CHAR except CTLs or tspecials>

          tspecials      = "(" | ")" | "<" | ">" | "@"
                         | "," | ";" | ":" | "\" | <">
                         | "/" | "[" | "]" | "?" | "="
                         | "{" | "}" | SP | HT

[anonymous]

This is a needless violation of Postel's Law ( ​http://en.wikipedia.org/wiki/Postel's_Law ). We, the users of Trac, congratulate you on your intimate knowledge of that particular RFC, but we also ask that you come to terms with the fact that our Tracs operate in an environment over which we do not necessarily have any control. In particular, other servers in our home domain may set malformed cookies, and this breaks Trac (for no good reason).

Please consider fixing.

[anonymous]

IIUC, this would need to be fixed in Python and not Trac.

[anonymous]

"We, the users of Trac…"

Be careful about speaking for the entire Trac user base.

[Christian Boos]

What we could do is sanitize the content of the cookie line 130​ before loading it, by removing the tspecials characters.

[koda]

after spending to night to install trac… I discover that it dont result of an erro of me. I'am realy sorry, trac is may be a good tool, but it sucks simply. When you consider that I can found tree "how to install trac on ubuntu" on the trac website, any of them want work. Each of them are an succession of "do that, or edit that and redo that but if your mother like apple dont edit this conf file…because me, Ihave replace that line by that once and…" and after all it is durty install. Dont you know "apt-get install trac" and nothing else. Is it too hard to produce good doc like gentoo doc. Is it too hard to provide an installer and independant system configuration procedure?

At the bigining I was thinking why python for a website? why not php. at the end I have the answer: simply because trac project is a big shit, and like the whole big shit it begin with a little shit.

thanks for waisting too happy days.

[Mark Rowe]

It's worth noting that WebSVN at times will set a cookie name of "/log.php". This causes Trac to throw the exception originally reported. I imagine that it's not particularly uncommon to want to host two SVN-related services under the same domain.

[Matthew Good]

I guess it's reasonable to try to ignore errors parsing cookies, though bug reports should be filed for the applications such as WebSVN that are setting these invalid cookies.

I've come up with a patch that overrides the cookie loading in order to skip the cookies that could not be loaded. Unfortunately this requires overloading private methods of the BaseCookie class in various non-obvious ways. The methods are patched before loading the cookies and then cleaned up afterwards to restore the standard behavior elsewhere in Trac. I'll post it here for comments:

trac/web/api.py

@@ -15 +15 @@
 # Author: Christopher Lenz <cmlenz@gmx.de>
 
 from BaseHTTPServer import BaseHTTPRequestHandler
-from Cookie import SimpleCookie as Cookie
+from Cookie import CookieError, SimpleCookie as Cookie
 import cgi
 import mimetypes
 import os
@@ -130 +130 @@
         self.incookie = Cookie()
         cookie = self.get_header('Cookie')
         if cookie:
+            old_set = self.incookie._BaseCookie__set
+            bad_cookies = []
+            def safe_set(key, real_value, coded_value):
+                try:
+                    old_set(key, real_value, coded_value)
+                except CookieError:
+                    bad_cookies.append(key)
+                    dict.__setitem__(self.incookie, key, None)
+            # override Cookie.set to ignore cookies with parse errors
+            self.incookie._BaseCookie__set = safe_set
+            # load the cookie values
             self.incookie.load(cookie)
+            # clean up the Cookie.set overriding
+            self.incookie._BaseCookie__set = old_set
+            for key in bad_cookies:
+                del self.incookie[key]
         self.outcookie = Cookie()
 
         self.base_url = self.environ.get('trac.base_url')

[Matthew Good]

#3527 has been marked as a duplicate.

Overriding private methods of the BaseCookie class is pretty ugly, but I'll put my previous patch up for review for 0.10 since this problem does seem to crop up from time to time.

[Matthew Good]

The Cookie module in Python 2.5 doesn't contain any changes that would break the above patch, so I think it should be safe to include unless someone knows a better way to implement this.

[Josh]

Does anyone have a patch for 0.9.6?

[Josh]

Here's the patch for source:/branches/0.9-stable/trac/web/modpython_frontend.py​. Basically it's the same as the patch above.

--- modpython_frontend.py.old   2006-09-08 12:20:02.000000000 -0400
+++ modpython_frontend.py       2006-09-08 12:24:01.000000000 -0400
@@ -28,6 +28,7 @@

 from mod_python import apache, util

+from Cookie import CookieError, SimpleCookie as Cookie
 from trac.util import http_date
 from trac.web.api import Request, RequestDone
 from trac.web.main import dispatch_request, get_environment, \
@@ -52,7 +53,25 @@
                 or self.server_port == 443:
             self.scheme = 'https'
         if self.req.headers_in.has_key('Cookie'):
-            self.incookie.load(self.req.headers_in['Cookie'])
+            #self.incookie.load(self.req.headers_in['Cookie'])
+           cookie = self.req.headers_in['Cookie']
+            old_set = self.incookie._BaseCookie__set
+            bad_cookies = []
+            def safe_set(key, real_value, coded_value):
+                try:
+                    old_set(key, real_value, coded_value)
+                except CookieError:
+                    bad_cookies.append(key)
+                    dict.__setitem__(self.incookie, key, None)
+            # override Cookie.set to ignore cookies with parse errors
+            self.incookie._BaseCookie__set = safe_set
+           # load the cookie values
+            self.incookie.load(cookie)
+            # clean up the Cookie.set overriding
+            self.incookie._BaseCookie__set = old_set
+            for key in bad_cookies:
+                del self.incookie[key]
+
         self.args = FieldStorageWrapper(self.req)

         # The root uri sometimes has to be explicitly specified because apache

[Josh]

Replying to Josh:

Here's the patch for source:/branches/0.9-stable/trac/web/modpython_frontend.py​. Basically it's the same as the patch above.

There was an extra space in the above patch. Here's a better one:

--- modpython_frontend.py.old   2006-09-08 12:20:02.000000000 -0400
+++ modpython_frontend.py       2006-09-08 12:24:01.000000000 -0400
@@ -28,6 +28,7 @@

 from mod_python import apache, util

+from Cookie import CookieError, SimpleCookie as Cookie
 from trac.util import http_date
 from trac.web.api import Request, RequestDone
 from trac.web.main import dispatch_request, get_environment, \
@@ -52,7 +53,25 @@
                 or self.server_port == 443:
             self.scheme = 'https'
         if self.req.headers_in.has_key('Cookie'):
-            self.incookie.load(self.req.headers_in['Cookie'])
+            #self.incookie.load(self.req.headers_in['Cookie'])
+            cookie = self.req.headers_in['Cookie']
+            old_set = self.incookie._BaseCookie__set
+            bad_cookies = []
+            def safe_set(key, real_value, coded_value):
+                try:
+                    old_set(key, real_value, coded_value)
+                except CookieError:
+                    bad_cookies.append(key)
+                    dict.__setitem__(self.incookie, key, None)
+            # override Cookie.set to ignore cookies with parse errors
+            self.incookie._BaseCookie__set = safe_set
+           # load the cookie values
+            self.incookie.load(cookie)
+            # clean up the Cookie.set overriding
+            self.incookie._BaseCookie__set = old_set
+            for key in bad_cookies:
+                del self.incookie[key]
+
         self.args = FieldStorageWrapper(self.req)

         # The root uri sometimes has to be explicitly specified because apache

[Matthew Good]

I've committed r3734 which moves the parsing workaround into a new subclass of SimpleCookie, so this should be fixed.