Opened 5 years ago

Closed 5 years ago

Last modified 4 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: master
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: UI/UX:

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 5 years ago.
Test illustrating the cookie issue
13007.diff (2.2 KB) - added by Ubercore 5 years ago.
Patch based on Trac's solution to the problem
13007.1.diff (3.0 KB) - added by ramiro 5 years ago.
Patch attached to trunk as of r15399

Download all attachments as: .zip

Change History (18)

comment:1 Changed 5 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

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

comment:2 Changed 5 years ago by Warlax

  • Resolution invalid deleted
  • Status changed from closed to 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 Changed 5 years ago by russellm

  • Resolution set to worksforme
  • Status changed from reopened to 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.

Changed 5 years ago by Ubercore

Test illustrating the cookie issue

comment:4 Changed 5 years ago by Ubercore

  • Patch needs improvement set
  • Resolution worksforme deleted
  • Status changed from closed to reopened
  • Version changed from 1.1 to 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.

http://bugs.python.org/issue2193

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

Changed 5 years ago by Ubercore

Patch based on Trac's solution to the problem

comment:5 Changed 5 years ago by Ubercore

  • 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 Changed 5 years ago by tttallis

  • Keywords sprintdec2010 added
  • milestone set to 1.3
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to 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 Changed 5 years ago by Ubercore

  • Cc Ubercore added

comment:8 Changed 5 years ago by russellm

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to 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.

Changed 5 years ago by ramiro

Patch attached to trunk as of r15399

comment:9 Changed 5 years ago by ramiro

  • 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 5 years ago by ramiro (previous) (diff)

comment:10 Changed 5 years ago by aaugustin

#7183 was a duplicate.

comment:11 Changed 5 years ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

comment:12 Changed 5 years ago by lukeplant

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 Changed 5 years ago by ramiro

  • Resolution set to fixed
  • Status changed from reopened to closed

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 Changed 5 years ago by ramiro

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 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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