Possible bug in messages framework?

47 views
Skip to first unread message

je...@jeffcroft.com

unread,
Jan 21, 2010, 11:31:46 PM1/21/10
to Django developers
Hey guys-

I've been using Django 1.2's new messages framework a lot lately, and
it's awesome. but, i think I may have discovered a bug (or possibly
I'm doing something wrong, but if so, I sure as hell can't figure out
what it is). It seems to me the messages are never delivered to the
user when there's an HttpResponseRedirect after the message is set.
For example:

def checkin(request, venue_id):
venue = get_object_or_404(Venue, id=venue_id)
checkin = CheckIn(singer=request.singer, venue=venue,
date_created=datetime.datetime.now())
checkin.save()
message = "Skadoosh! You're checked in to %s." % venue.name
messages.success(request, message)
return HttpResponseRedirect(request.META['HTTP_REFERER'])

This doesn't work. When the user is redirected, they don't see the
message. On the other hand, this works fine:

def checkin(request, venue_id):
venue = get_object_or_404(Venue, id=venue_id)
checkin = CheckIn(singer=request.singer, venue=venue,
date_created=datetime.datetime.now())
checkin.save()
message = "Skadoosh! You're checked in to %s." % venue.name
messages.success(request, message)
return render_to_response("path/to/template.html", { 'message':
message }, context_instance=RequestContext(request))

This is all using the default backend (LegacyFallbackStorage).

Thoughts? Bug, or am I doing something wrong? Thanks in advance, all!

je...@jeffcroft.com

unread,
Jan 21, 2010, 11:55:13 PM1/21/10
to Django developers
After a little more playing around, I've discovered that this is not
an issue if I use the session storage -- so it seems to be related to
cookie storage.

Sean Brant

unread,
Jan 22, 2010, 12:07:12 AM1/22/10
to django-d...@googlegroups.com
I wonder if this is related?
http://groups.google.com/group/django-developers/browse_thread/thread/5613e9a03d92c902/738a3b81e405dc78#738a3b81e405dc78

> --
> You received this message because you are subscribed to the Google Groups "Django developers" group.
> To post to this group, send email to django-d...@googlegroups.com.
> To unsubscribe from this group, send email to django-develop...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
>

je...@jeffcroft.com

unread,
Jan 22, 2010, 12:13:04 AM1/22/10
to Django developers
Sean-

Can't say for sure it's related, but I can verify that I was using
Safari (Mobile) and that my message had commas in them...so it
definitely could be.

Switching to session storage solved the problem for me, and that
backend works fine for my needs. So, it's no longer pressing issue
for me, but there definitely seems to be something wrong in the cookie
storage backend.

Jeff

On Jan 21, 9:07 pm, Sean Brant <brant.s...@gmail.com> wrote:
> I wonder if this is related?http://groups.google.com/group/django-developers/browse_thread/thread...

Tobias McNulty

unread,
Jan 22, 2010, 8:23:05 AM1/22/10
to django-d...@googlegroups.com
Hi Jeff,

Could you try again without a comma in the message and see if the CookieStorage starts working again?  Either way, that definitely sounds like a bug to me.

Thanks,
Tobias
--
Tobias McNulty
Caktus Consulting Group, LLC
P.O. Box 1454
Carrboro, NC 27510
(919) 951-0052
http://www.caktusgroup.com

SmileyChris

unread,
Jan 22, 2010, 1:53:15 PM1/22/10
to Django developers
Looking around, this looks like a problem for other frameworks too
(see http://trac.turbogears.org/ticket/1164)
If we're to accept that turbogears example, it sounds like we're not
properly encoding the cookie in core, rather than patching messages.

On Jan 23, 2:23 am, Tobias McNulty <tob...@caktusgroup.com> wrote:
> Hi Jeff,
>
> Could you try again without a comma in the message and see if the
> CookieStorage starts working again?  Either way, that definitely sounds like
> a bug to me.
>
> Thanks,
> Tobias
>

> On Fri, Jan 22, 2010 at 12:13 AM, j...@jeffcroft.com <j...@jeffcroft.com>wrote:
>
>
>
>
>
> > Sean-
>
> > Can't say for sure it's related, but I can verify that I was using
> > Safari (Mobile) and that my message had commas in them...so it
> > definitely could be.
>
> > Switching to session storage solved the problem for me, and that
> > backend works fine for my needs. So, it's no longer  pressing issue
> > for me, but there definitely seems to be something wrong in the cookie
> > storage backend.
>
> > Jeff
>
> > On Jan 21, 9:07 pm, Sean Brant <brant.s...@gmail.com> wrote:
> > > I wonder if this is related?
> >http://groups.google.com/group/django-developers/browse_thread/thread...
>
> > > On Jan 21, 2010, at 10:55 PM, j...@jeffcroft.com wrote:
>
> > > > After a little more playing around, I've discovered that this is not
> > > > an issue if I use the session storage -- so it seems to be related to
> > > > cookie storage.
>
> > > > --
> > > > You received this message because you are subscribed to the Google
> > Groups "Django developers" group.
> > > > To post to this group, send email to
> > django-d...@googlegroups.com.
> > > > To unsubscribe from this group, send email to

> > django-develop...@googlegroups.com<django-developers%2Bunsubscr i...@googlegroups.com>


> > .
> > > > For more options, visit this group athttp://
> > groups.google.com/group/django-developers?hl=en.
>
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Django developers" group.
> > To post to this group, send email to django-d...@googlegroups.com.
> > To unsubscribe from this group, send email to

> > django-develop...@googlegroups.com<django-developers%2Bunsubscr i...@googlegroups.com>

Vinay Sajip

unread,
Jan 22, 2010, 2:20:20 PM1/22/10
to Django developers
On Jan 22, 1:53 pm, SmileyChris <smileych...@gmail.com> wrote:
> If we're to accept that turbogears example, it sounds like we're not
> properly encoding the cookie in core, rather than patching messages.

Yes, it appears to be a matter of luck that other browsers accept
cookie values which are invalid according to the cookie spec. This
comment on a Webkit bug has useful information:

https://bugs.webkit.org/show_bug.cgi?id=6063#c7

Regards,

Vinay Sajip

Luke Plant

unread,
Jan 22, 2010, 8:04:26 PM1/22/10
to django-d...@googlegroups.com

Well, it depends on what you call the 'spec'. What spec says that
commas in values is invalid?

The 'spec' linked to on that WebKit bug is a preliminary Netscape
document, which, as far as I can tell, eventually turned into RFC
2109, which surely has got to be regarded as more authoritative. RFC
2965 (which proposes Set-Cookie2) supposedly obsoletes RFC 2109, but I
don't know think it is really used much. [1]

As I noted on the bug [2], RFC 2109 allows values to be quoted, in
which case Django is behaving correctly, and it is some browsers that
are not.

Our implementation of this is in fact done entirely by Python's
standard library Cookie module [3]. It handles everything I can throw
at it (newlines, UTF8, throws exceptions with illegal cookie names
etc.). It's kind of unlikely that we've found a bug in it.

Of course, it doesn't mean we shouldn't fix things to avoid this bug.
But to do so would require some kind of encoding, which is almost
certainly going to cause breakage of some kind with existing cookies.
Turbogears' solution would be backwards compatible in most cases, but
not all.

(BTW, if we implemented this change, the nicer way to do is to
subclass Cookie and override Cookie.value_encode() and value_decode(),
rather than the way that Turbogears does it)

Personally, I favour fixing our messages implementation so that it
isn't an issue (which is easy, it seems, see details on #12470), and
possibly just putting a note into our cookie documentation that some
old WebKit based browsers have a bug that means they do not correctly
handle a cookie value containing a comma followed by a space.

An argument in favour of this lazy approach is that this issue, for
both ourselves and Turbogears, has only ever come up in the context of
using cookies for messages. Presumably that means that developers are
rarely storing extended human readable text strings in cookies outside
of this kind of usage, so outside of the messages app it is probably
not something we need to worry about.

Luke

[1] http://code.google.com/webstats/2005-12/httpheaders.html
[2] http://code.djangoproject.com/ticket/12470#comment:4
[3] http://docs.python.org/library/cookie.html

--
Sometimes I wonder if men and women really suit each other. Perhaps
they should live next door and just visit now and then. (Katherine
Hepburn)

Luke Plant || http://lukeplant.me.uk/

Sean Brant

unread,
Jan 22, 2010, 8:20:55 PM1/22/10
to django-d...@googlegroups.com


Whats the downside of fixing this at the core cookie handling level? I agree with Luke and only ran across this bug when the new messaging framework dropped. However if we are going to fix the problem, and I do think it's a problem even if its a browser bug, should we just fix it at the core level and handle all cookies down the road? Would previously stored cookies that are not url quoted even fail when trying to unquote? Maybe I'm wrong but this seems pretty backwards compatible.

Sean

Luke Plant

unread,
Jan 22, 2010, 9:44:39 PM1/22/10
to django-d...@googlegroups.com
On Saturday 23 January 2010 01:20:55 Sean Brant wrote:

> Whats the downside of fixing this at the core cookie handling
> level? I agree with Luke and only ran across this bug when the new
> messaging framework dropped. However if we are going to fix the
> problem, and I do think it's a problem even if its a browser bug,
> should we just fix it at the core level and handle all cookies
> down the road? Would previously stored cookies that are not url
> quoted even fail when trying to unquote? Maybe I'm wrong but this
> seems pretty backwards compatible.

It's true that the vast majority of existing cookie values will be
interpreted the same whether you URL-unquote or not. i.e.

unquote_cookie(value) == value

in many cases so this isn't a big deal. But it's not *always* true,
otherwise there is no need for unquote_cookie. And every time it's
not true is a potential bug with previously stored cookies. The most
likely scenario I can think of is if a cookie is being used to store
some query string or previous URL (like a saved search), which might
then be used literally in some way (e.g. as the parameter to
HttpResponseRedirect). By URL unquoting, when we didn't before, we
would introduce a bug.

e.g.
HttpResponseRedirect("http://foo.com/?q=fr%C3%A8re")

is not the same as

HttpResponseRedirect("http://foo.com/?q=fr\xc3\xa8re")

(the latter throws an exception in this case).

BTW, Turbogears' solution is actually buggy for some input:

>>> assert unquote_cookie(quote_cookie("%25")) == "%25"
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AssertionError

Coming up with your own encoding is sometimes tricker than it looks.

I don't think this is a big deal because, either way, I don't think
many people are going to be affected.

BTW, further research shows that we are not really RFC 2109 compliant
at all, but then again no-one is. It seems virtually everyone (server
side and client side) is using 'Netscape style' cookies with some
things adopted from RFC 2109 and RFC 2965, including 'max-age' and the
use of quoted-string, but not the all important "Version" attribute
which turns on RFC 2109 cookies. Hardly anyone is using Set-Cookie2
from RFC 2965. So specs of any kind are fairly meaningless here, it's
a matter of what everyone does.


Luke

Luke Plant

unread,
Jan 22, 2010, 10:45:52 PM1/22/10
to django-d...@googlegroups.com
On Saturday 23 January 2010 02:44:39 Luke Plant wrote:

> BTW, further research shows that we are not really RFC 2109
> compliant at all, but then again no-one is. It seems virtually
> everyone (server side and client side) is using 'Netscape style'
> cookies with some things adopted from RFC 2109 and RFC 2965,
> including 'max-age' and the use of quoted-string, but not the all
> important "Version" attribute which turns on RFC 2109 cookies.
> Hardly anyone is using Set-Cookie2 from RFC 2965. So specs of any
> kind are fairly meaningless here, it's a matter of what everyone
> does.

Actually, to add a bit more:

http://www.ietf.org/mail-archive/web/http-state/current/msg00078.html
http://codereview.chromium.org/17045

It's all pretty horrific, it pushes me back towards adding a layer of
quoting to our cookie handling just to try to avoid it all - but a
robust encoding which definitely avoids all problems. We should note
that the presence of semi-colons is more likely to cause problems than
commas - Internet Explorer splits on semi-colons, irrespective of
quotation marks.

Tobias McNulty

unread,
Jan 23, 2010, 9:29:55 AM1/23/10
to django-d...@googlegroups.com
On Fri, Jan 22, 2010 at 10:45 PM, Luke Plant <L.Pla...@cantab.net> wrote:
On Saturday 23 January 2010 02:44:39 Luke Plant wrote:

>  BTW, further research shows that we are not really RFC 2109
>  compliant at all, but then again no-one is.  It seems virtually
>  everyone (server side and client side) is using 'Netscape style'
>  cookies with some things adopted from RFC 2109 and RFC 2965,
>  including 'max-age' and the use of quoted-string, but not the all
>  important "Version" attribute which turns on RFC 2109 cookies.
>  Hardly anyone is using Set-Cookie2 from RFC 2965.  So specs of any
>  kind are fairly meaningless here, it's a matter of what everyone
>  does.

Actually, to add a bit more:

http://www.ietf.org/mail-archive/web/http-state/current/msg00078.html
http://codereview.chromium.org/17045

It's all pretty horrific, it pushes me back towards adding a layer of
quoting to our cookie handling just to try to avoid it all - but a
robust encoding which definitely avoids all problems.  We should note
that the presence of semi-colons is more likely to cause problems than
commas - Internet Explorer splits on semi-colons, irrespective of
quotation marks.

Technically I imagine it is possible to come up with a way to encode all new cookies in a safe way, but still support "decoding" old-style cookies.  That said, I have reservations about any kind of across-the-board encoding because it makes it necessary, when/if the cookies need to be read by JavaScript, to implement that same decode/encode on the client side.  My personal preference would be to fix the messages implementation and add a note to the cookies documentation saying that it's "recommended to encode cookies to avoid potential browser bugs," and list off a few of those bugs.

Tobias

Luke Plant

unread,
Jan 23, 2010, 12:21:01 PM1/23/10
to django-d...@googlegroups.com
On Saturday 23 January 2010 14:29:55 Tobias McNulty wrote:

> That said, I have reservations about any kind
> of across-the-board encoding because it makes it necessary,
> when/if the cookies need to be read by JavaScript, to implement
> that same decode/encode on the client side.

We actually already encode many values - the SimpleCookie module does
it for us:

>>> c = Cookie.SimpleCookie()
>>> c['test'] = 'He said "Voilà!"'
>>> c.output()
'Set-Cookie: test="He said \\"Voil\\303\\240!\\""'

Robust Javascript that uses cookies with any 'funny' characters
already needs to be able to cope with these octal sequences, with
backslashes for quotation marks, and the fact that quotation marks are
added for any values with special chars (including space, comma and
semi-colon).

So, one method that would minimize damage would be to extend the
existing octal escape mechanism to semi-colons and commas. That way,
javascript only needs to implement one unquoting mechanism. I've
implemented this and added the patch to #12470.

This still has the disadvantage that if people are storing comma or
semi-colon separated lists in a cookie, and are reading that cookie
from javascript, and haven't fully implemented the reverse quoting for
our existing cookie quoting, then they will have problems.

Personally, I imagine that very few Django projects are storing
complex values in cookies in Django - most people would just put stuff
in the session, or do some AJAX these days.

If people *are* storing complex values, I imagine that many will be
using base64 or something (it's very easy with builtin javascript
functions atob, btoa).

So, currently I'm inclined towards committing the patch I just added
to #12470, and adding a note in 'backwards incompatibilities'.

Thoughts?

Luke

--
The early bird gets the worm, but the second mouse gets the cheese.
--Steven Wright

Luke Plant || http://lukeplant.me.uk/

Tobias McNulty

unread,
Jan 23, 2010, 2:02:06 PM1/23/10
to django-d...@googlegroups.com
On Sat, Jan 23, 2010 at 12:21 PM, Luke Plant <L.Pla...@cantab.net> wrote:
So, currently I'm inclined towards committing the patch I just added
to #12470, and adding a note in 'backwards incompatibilities'.

Looks good to me.

Tobias

Luke Plant

unread,
Jan 23, 2010, 2:29:59 PM1/23/10
to django-d...@googlegroups.com
On Saturday 23 January 2010 19:02:06 Tobias McNulty wrote:
> On Sat, Jan 23, 2010 at 12:21 PM, Luke Plant <L.Pla...@cantab.net>
wrote:
> > So, currently I'm inclined towards committing the patch I just
> > added to #12470, and adding a note in 'backwards
> > incompatibilities'.
>
> Looks good to me.

The encoding issue, with or without my patch, actually brings up the
issue of a small bug in your messages code :-)

CookieStorage assumes that the length of the 'encoded' data (i.e. the
return val from CookieStorage._encode()) is the number of bytes that
the data takes up in the cookie. But the additional encoding that
SimpleCookie does means you actually have less space, depending on the
data.

The simplest solution I can think of would be to base64 the whole
value, as already suggested for other reasons. SimpleCookie would
then have no need to change any of it, and the assumption would hold.

Tobias McNulty

unread,
Jan 23, 2010, 3:21:01 PM1/23/10
to django-d...@googlegroups.com
On Sat, Jan 23, 2010 at 2:29 PM, Luke Plant <L.Pla...@cantab.net> wrote:
The encoding issue, with or without my patch, actually brings up the
issue of a small bug in your messages code :-)

CookieStorage assumes that the length of the 'encoded' data (i.e. the
return val from CookieStorage._encode()) is the number of bytes that
the data takes up in the cookie.  But the additional encoding that
SimpleCookie does means you actually have less space, depending on the
data.

The simplest solution I can think of would be to base64 the whole
value, as already suggested for other reasons.  SimpleCookie would
then have no need to change any of it, and the assumption would hold.

Good catch - here's a patch with the proposed fix:

http://code.djangoproject.com/attachment/ticket/12470/12670_messages_base64.diff

This incidentally fixes the original bug, too, regardless of the decision about escaping ',' and ';' in the core.  It does have the downside, however, of a 33% increase in the message cookie size.

Tobias

Luke Plant

unread,
Jan 23, 2010, 5:05:08 PM1/23/10
to django-d...@googlegroups.com
On Saturday 23 January 2010 20:21:01 Tobias McNulty wrote:

> Good catch - here's a patch with the proposed fix:
>
> http://code.djangoproject.com/attachment/ticket/12470/12670_message
> s_base64.diff
>
> This incidentally fixes the original bug, too, regardless of the
> decision about escaping ',' and ';' in the core. It does have the
> downside, however, of a 33% increase in the message cookie size.

Hmm, that's a significant decrease. I guess the alternative is to use
CompatCookie().value_encode() inside CookieStorage._storage() to work
out how much space the encoded data will eventually take up. It's
more fragile, but maybe a good trade off.

Luke Plant

unread,
Jan 23, 2010, 5:23:54 PM1/23/10
to django-d...@googlegroups.com
While on the subject - I just found out that IE6 and IE7 impose a
limit of 4096 bytes *total* for cookies from a domain [1]. With the
current max_cookie_size, we leave no room for other cookies, which is
bad. Maybe we should reduce to, say 3/4 of 4K.

Luke

[1] http://blogs.neoseeker.com/Redemption/542-browser-cookies-
limitations-ie6-ie7-others/

Tobias McNulty

unread,
Jan 23, 2010, 5:39:43 PM1/23/10
to django-d...@googlegroups.com
On Sat, Jan 23, 2010 at 5:23 PM, Luke Plant <L.Pla...@cantab.net> wrote:
While on the subject - I just found out that IE6 and IE7 impose a
limit of 4096 bytes *total* for cookies from a domain [1].  With the
current max_cookie_size, we leave no room for other cookies, which is
bad.  Maybe we should reduce to, say 3/4 of 4K.

Wow, that's unfortunate.  Should we add some sort of compression while we're at it?

Luke Plant

unread,
Jan 23, 2010, 5:49:29 PM1/23/10
to django-d...@googlegroups.com
On Saturday 23 January 2010 22:39:43 Tobias McNulty wrote:

> > While on the subject - I just found out that IE6 and IE7 impose a
> > limit of 4096 bytes *total* for cookies from a domain [1]. With
> > the current max_cookie_size, we leave no room for other cookies,
> > which is bad. Maybe we should reduce to, say 3/4 of 4K.
>
> Wow, that's unfortunate. Should we add some sort of compression
> while we're at it?

Is it worth it at this point? I imagine in most instances a message
won't be more than a few 100 characters, and you would only have at
most a handful of messages sent to the user at once. It's rare to be
pushing 1000s of characters around, and that's what the
FallbackStorage is for.

Luke

Tobias McNulty

unread,
Jan 23, 2010, 6:09:49 PM1/23/10
to django-d...@googlegroups.com
On Sat, Jan 23, 2010 at 5:49 PM, Luke Plant <L.Pla...@cantab.net> wrote:
Is it worth it at this point?  I imagine in most instances a message
won't be more than a few 100 characters, and you would only have at
most a handful of messages sent to the user at once.  It's rare to be
pushing 1000s of characters around, and that's what the
FallbackStorage is for.

Yeah, probably not.  Upon further investigation it seems like most of the compression algorithms built in to python actually increase the size of the text for short strings, and that's before the 33% base64 increase.

je...@jeffcroft.com

unread,
Jan 25, 2010, 3:16:10 AM1/25/10
to Django developers
I just want to say that everything you guys are talking about is so
totally over my head, but I'm stoked you're working it out. Thanks,
guys. :)

Luke Plant

unread,
Jan 25, 2010, 12:37:07 PM1/25/10
to django-d...@googlegroups.com

:) It should be fixed now, I committed the relevant changes on
Saturday. If it's still not working, let us know on this bug:

http://code.djangoproject.com/ticket/12470

Cheers,

Tobias McNulty

unread,
Jan 25, 2010, 12:50:45 PM1/25/10
to django-d...@googlegroups.com
I created a ticket to track this bug at http://code.djangoproject.com/ticket/12686


On Sat, Jan 23, 2010 at 5:23 PM, Luke Plant <L.Pla...@cantab.net> wrote:
While on the subject - I just found out that IE6 and IE7 impose a
limit of 4096 bytes *total* for cookies from a domain [1].  With the
current max_cookie_size, we leave no room for other cookies, which is
bad.  Maybe we should reduce to, say 3/4 of 4K.

Luke

[1] http://blogs.neoseeker.com/Redemption/542-browser-cookies-
limitations-ie6-ie7-others/



--

je...@jeffcroft.com

unread,
Jan 25, 2010, 1:11:45 PM1/25/10
to Django developers
Indeed it is fixed. Thanks so much, guys!

On Jan 25, 9:37 am, Luke Plant <L.Plant...@cantab.net> wrote:

Reply all
Reply to author
Forward
0 new messages