Possible bug in messages framework?
je...@jeffcroft.com
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
an issue if I use the session storage -- so it seems to be related to
cookie storage.
Sean Brant
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
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
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
(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
> 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
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
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
> 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
> 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
On Saturday 23 January 2010 02:44:39 Luke Plant wrote:Actually, to add a bit more:
> 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.
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
> 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
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
> 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
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
> 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
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
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 Plant
> > 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
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.
je...@jeffcroft.com
totally over my head, but I'm stoked you're working it out. Thanks,
guys. :)
Luke Plant
:) 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
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
On Jan 25, 9:37 am, Luke Plant <L.Plant...@cantab.net> wrote: