#13007 closed (fixed)
Django fails to log in when a cookie is set on the same domain containing a colon
Reported by: | Warlax | Owned by: | nobody |
---|---|---|---|
Component: | HTTP handling | Version: | dev |
Severity: | Keywords: | cookies, sprintdec2010 | |
Cc: | Ubercore | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
From the release notes for the development version of django 1.2 I found this.
"""
Cookie encoding
To fix bugs with cookies in Internet Explorer, Safari, and possibly other browsers, our encoding of cookie values was changed so that the characters comma and semi-colon are treated as non-safe characters, and are therefore encoded as \054 and \073 respectively. This could produce backwards incompatibilities, especially if you are storing comma or semi-colon in cookies and have javascript code that parses and manipulates cookie values client-side.
"""
One bug we have found through using version 1.0 and 1.1 is that django also fails to understand cookies with a ":" in their name. This makes the login system completely fall over, so it mite be worth encoding those too. We discovered this bug when running django under the same domain as another tool, and it was settings cookies such as blah:wiki, and once those cookies were set upon that domain, we simply could not log back into the django section of the site without first clearing the offending cookies.
Hopefully I have provided enough information to help you fix this bug.
Ash
Attachments (3)
Change History (18)
comment:1 by , 15 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
comment:2 by , 15 years ago
Resolution: | invalid |
---|---|
Status: | closed → reopened |
Create a standard django app with the default auth system
create a view that is @login_required
MANUALLY create a cookie in ur browser under the url django is servered at, ie 127.0.0.1:8000
create this cookie
"org.ditchnet.jsp.tabs:task-details" with a value of anything, eg "231432423"
try and log into the django app. you will not be able to view the page behind the login required decorator
comment:3 by , 15 years ago
Resolution: | → worksforme |
---|---|
Status: | reopened → closed |
I can't reproduce with the provided instructions. Like I said, a colon should be a valid value for a cookie. A programatic test case might help.
comment:4 by , 14 years ago
Patch needs improvement: | set |
---|---|
Resolution: | worksforme |
Status: | closed → reopened |
Version: | 1.1 → SVN |
I've added a test case to illustrate what I think is going on here. The culprit in my case was Glassfish running on the same server. Its admin console adds this cookie:
form:tree-hi=;
This breaks cookie parsing, and no cookies appear in the request. Including the csrf token. I think the ideal case here is to lose only the non-standard cookies, instead of returning a blank dict when a CookieError is raised. This is, I think, what Trac has done.
comment:5 by , 14 years ago
Has patch: | set |
---|
Without rolling some kind of cookie pre-processing, Trac's solution seems like a decent option until Python provides better hooks for error handling on individual keys. I've attached a patch based on http://trac.edgewall.org/browser/trunk/trac/web/api.py?rev=3734#L97.
comment:6 by , 14 years ago
Keywords: | sprintdec2010 added |
---|---|
milestone: | → 1.3 |
Patch needs improvement: | unset |
Triage Stage: | Unreviewed → Ready for checkin |
Spent a bit of time reviewing this ticket during the sprint.
A few points:
- While "A colon *should* be a valid value in a cookie", this ticket is actually about cookies with a colon in the name. While there is some uncertainty about what characters are legal in cookie names, most folks seem to thing colons aren't kosher.
- In any case, I followed the instructions, and with some fiddling was able to reproduce the problem. I failed to reproduce it in Safari (it looked like maybe Safari 'censored' the cookie with the colon), but managed to definitively reproduce it in Firefox.
- The patch takes a reasonable, prudent approach, and solves the issue cited
- The unittest isn't exactly conclusive but I can't think of a better way to test this - it's not exactly an easy situation to reproduce
I say let check it in!
comment:7 by , 14 years ago
Cc: | added |
---|
comment:8 by , 14 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
This patch doesn't apply clean to trunk, and it's not obvious how to adapt the patch -- there have been other modifications to the cookie code in the interim.
comment:9 by , 14 years ago
Patch needs improvement: | unset |
---|
I've updated the patch to apply after r15298. Please review, particularly:
- Replaced a explicit
SimpleCookie.load(self, rawdata)
in the added load() method with asuper(SimpleCookie, self).load(rawdata)
call - Added a runtime
_cookie_allows_colon_in_names
flag in the same spirit of the_morsel_supports_httponly
and_cookie_encodes_correctly
already there.
comment:11 by , 14 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
A colon *should* be a valid value in a cookie; you'll need to provide a specific failing test case.