Opened 14 years ago

Closed 13 years ago

Last modified 12 years ago

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

13007_testcase.diff (955 bytes ) - added by Ubercore 13 years ago.
Test illustrating the cookie issue
13007.diff (2.2 KB ) - added by Ubercore 13 years ago.
Patch based on Trac's solution to the problem
13007.1.diff (3.0 KB ) - added by Ramiro Morales 13 years ago.
Patch attached to trunk as of r15399

Download all attachments as: .zip

Change History (18)

comment:1 by Russell Keith-Magee, 14 years ago

Resolution: invalid
Status: newclosed

A colon *should* be a valid value in a cookie; you'll need to provide a specific failing test case.

comment:2 by Warlax, 14 years ago

Resolution: invalid
Status: closedreopened

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 Russell Keith-Magee, 14 years ago

Resolution: worksforme
Status: reopenedclosed

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.

by Ubercore, 13 years ago

Attachment: 13007_testcase.diff added

Test illustrating the cookie issue

comment:4 by Ubercore, 13 years ago

Patch needs improvement: set
Resolution: worksforme
Status: closedreopened
Version: 1.1SVN

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.

http://bugs.python.org/issue2193

http://trac.edgewall.org/ticket/2256

by Ubercore, 13 years ago

Attachment: 13007.diff added

Patch based on Trac's solution to the problem

comment:5 by Ubercore, 13 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 Thomas Ashelford, 13 years ago

Keywords: sprintdec2010 added
milestone: 1.3
Patch needs improvement: unset
Triage Stage: UnreviewedReady 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 Ubercore, 13 years ago

Cc: Ubercore added

comment:8 by Russell Keith-Magee, 13 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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.

by Ramiro Morales, 13 years ago

Attachment: 13007.1.diff added

Patch attached to trunk as of r15399

comment:9 by Ramiro Morales, 13 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 a super(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.
Last edited 13 years ago by Ramiro Morales (previous) (diff)

comment:10 by Aymeric Augustin, 13 years ago

#7183 was a duplicate.

comment:11 by Jannis Leidel, 13 years ago

Triage Stage: AcceptedReady for checkin

comment:12 by Luke Plant, 13 years ago

Just a small note - 13007.1.diff is absolutely the right way to do it in trunk, but it will be a pain to backport to 1.2.X, due to [15298] and the fact that [14707] is a feature addition which is not backported to 1.2.X. So it looks like 13007.1.diff should be applied to trunk, 13007.diff to 1.2.X.

comment:13 by Ramiro Morales, 13 years ago

Resolution: fixed
Status: reopenedclosed

In [15523]:

Fixed #13007 -- Made cookie parsing resilent to the presence of cookies with invalid characters in their names. Thanks Warlax for the report, Ubercore for his work on a fix and Jannis and Luke for review and guidance.

comment:14 by Ramiro Morales, 13 years ago

In [15524]:

[1.2.X] Fixed #13007 -- Made cookie parsing resilent to the presence of cookies with invalid characters in their names. Thanks Warlax for the report, Ubercore for his work on a fix and Jannis and Luke for review and guidance.

Backport of [15523] from trunk.

comment:15 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

Note: See TracTickets for help on using tickets.
Back to Top