Edgewall Software
Modify

Opened 18 years ago

Closed 18 years ago

#2256 closed defect (fixed)

Wrong cookie is retrieved when loading trac

Reported by: aroach@… Owned by: Matthew Good
Priority: normal Milestone: 0.10
Component: general Version: 0.8.4
Severity: major Keywords: review
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description

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.

Attachments (0)

Change History (14)

comment:1 by Matthew Good, 18 years ago

Milestone: 0.9
Resolution: wontfix
Status: newclosed

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

comment:2 by anonymous, 18 years ago

Resolution: wontfix
Status: closedreopened

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.

comment:3 by anonymous, 18 years ago

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

comment:4 by anonymous, 18 years ago

"We, the users of Trac…"

Be careful about speaking for the entire Trac user base.

comment:5 by Christian Boos, 18 years ago

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

comment:6 by koda, 18 years ago

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.

comment:7 by Mark Rowe, 18 years ago

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.

comment:8 by Matthew Good, 18 years ago

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

     
    1515# Author: Christopher Lenz <cmlenz@gmx.de>
    1616
    1717from BaseHTTPServer import BaseHTTPRequestHandler
    18 from Cookie import SimpleCookie as Cookie
     18from Cookie import CookieError, SimpleCookie as Cookie
    1919import cgi
    2020import mimetypes
    2121import os
     
    130130        self.incookie = Cookie()
    131131        cookie = self.get_header('Cookie')
    132132        if cookie:
     133            old_set = self.incookie._BaseCookie__set
     134            bad_cookies = []
     135            def safe_set(key, real_value, coded_value):
     136                try:
     137                    old_set(key, real_value, coded_value)
     138                except CookieError:
     139                    bad_cookies.append(key)
     140                    dict.__setitem__(self.incookie, key, None)
     141            # override Cookie.set to ignore cookies with parse errors
     142            self.incookie._BaseCookie__set = safe_set
     143            # load the cookie values
    133144            self.incookie.load(cookie)
     145            # clean up the Cookie.set overriding
     146            self.incookie._BaseCookie__set = old_set
     147            for key in bad_cookies:
     148                del self.incookie[key]
    134149        self.outcookie = Cookie()
    135150
    136151        self.base_url = self.environ.get('trac.base_url')

comment:9 by Matthew Good, 18 years ago

Keywords: review added
Milestone: 0.10
Owner: changed from Jonas Borgström to Matthew Good
Status: reopenednew

#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.

comment:10 by Matthew Good, 18 years ago

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.

comment:11 by Josh, 18 years ago

Does anyone have a patch for 0.9.6?

comment:12 by Josh, 18 years ago

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

in reply to:  12 comment:13 by Josh, 18 years ago

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

comment:14 by Matthew Good, 18 years ago

Resolution: fixed
Status: newclosed

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Matthew Good.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Matthew Good to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.