Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#12470 closed (fixed)

django.contrib.messages CookieStorage failing silently in safari when comma is used in message

Reported by: Sean Brant Owned by: nobody
Component: Uncategorized Version: 1.1
Severity: Keywords:
Cc: Michael Malone Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

f you try and send a message with new messaging app that contains a
comma in the message it does not work in Safari.

I'm not really sure why the json encoded message is not working in the
Safari browser, but I fixed it by adding a base64 encode and decode
step that encodes/decodes the message string.

Attachments (4)

base64_fix_cookie.diff (458 bytes ) - added by Sean Brant 14 years ago.
12470-cookie-fix.diff (3.5 KB ) - added by Luke Plant 14 years ago.
Patch that fixes this at the level of cookie handling
12470-cookie-fix-2.diff (4.2 KB ) - added by Luke Plant 14 years ago.
Added release notes.
12670_messages_base64.diff (2.8 KB ) - added by Tobias McNulty 14 years ago.
base64-encode message payload in CookieStorage to escape special characters and reliably predict cookie size

Download all attachments as: .zip

Change History (12)

by Sean Brant, 14 years ago

Attachment: base64_fix_cookie.diff added

comment:1 by Sean Brant, 14 years ago

For some reason the diff does not show up when you preview the file but works when you download it...

comment:2 by Luke Plant, 14 years ago

This looks relevant: http://lists.macosforge.org/pipermail/webkit-unassigned/2006-March/006475.html

As does the spec for cookies, which implies that the cookie value should not contain a comma or semi-colon for safety.

An alternative way to fix this is to fix our cookie handling so that it encodes any potentially tricky characters. The problem here is backwards compatibility - existing cookies will all stop working if we change this.

However, one possibility would be to use something like URL percent encoding. This would mean that many existing cookies are not broken - if they don't contain a % char, they will be fine.

It is probably already the case that most usages of cookies don't use 'funny' chars like ',' and ';', otherwise people would fall foul of bugs like this. People who are storing arbitrary strings in cookies are probably already using some kind of encoding. With many standard encodings (e.g. base64), they will be fine with the proposed added percent encoding. But obviously if people are already using percent encoding, there will be problems.

At the moment I'm inclined to go with your base64 encoding fix, and possibly add percent encoding to our cookie handling after some discussion.

BTW, your diff probably doesn't get shown because it is not a unified diff (which is the norm).

comment:3 by Tobias McNulty, 14 years ago

Sean, does your patch work if multiple messages are created during the same request? Unless I'm missing something, if 2+ messages exist, the JSON will also contain a comma and also break the cookie handling somewhere along the line. Perhaps the entire JSON-encoded string should be base64-encoded.

comment:4 by Luke Plant, 14 years ago

Hmm, RFC 2109, as opposed to other sources, actually says that the "value" of the cookie can be a "quoted-string". According to RFC 2608, that is defined as follows:

   A string of text is parsed as a single word if it is quoted using
   double-quote marks.

          quoted-string  = ( <"> *(qdtext) <"> )

          qdtext         = <any TEXT except <">>

   The backslash character ("\") may be used as a single-character quoting
   mechanism only within quoted-string and comment constructs.

          quoted-pair    = "\" CHAR

This means that we can have any char, including a comma, if we quote the whole string. Which is exactly what Django's cookie handling already does. A typical view that sets a message produces output like this:

   Set-Cookie:  messages="ead4512992ebb2255d2f238a1bc9a3def8a3e657$[[\"__json_message\",20,\"This, is a dramatic message.\"]]"; Path=/

So it contains a comma whether or not the message does, but the whole thing is quoted, with quotation marks correctly escaped with a backslash.

I've tested some browsers with this, and found the following:

  • Firefox (Linux), Safari 4 (Windows), Chrome (Linux): all are "correct", accepting a comma in cookie value provided it is in a quoted string, and in some other circumstances. All correctly handle quote escaping.
  • Safari 3 (Windows): drops everything after the comma, if the comma is followed by a space, irrespective of whether the value is a quoted string or not.

Obviously this WebKit bug has been fixed in Chrome and Safari 4. The Safari 3 behaviour explains why we are only getting the bug if the message contains a comma, because the comma in a human presentable string is typically followed by a space.

Phew! That took some tracking down.

The next question is: what do we do about it? I consider this a browser bug, but it's one that affects us. The base64 fix does indeed work, given the nature of this bug, and it doesn't require adding the fix into our cookie setting code. So I propose going with that, unless someone has a more compelling solution.

Obviously, people setting cookies manually may still be hit by this bug, but that is their problem, we can't fix every browser bug.

comment:5 by Luke Plant, 14 years ago

After further discussion/research, the RFCs are basically useless here, as none of the browsers actually follow any RFC correctly (with the possible exception of Opera). Internet Explorer is also possibly 'compliant', but only attempts 'Netscape style cookies', which don't support quoted-string. That means it splits on semi-colons, and values like "hello;goodbye" will break. Safari and Chrome follow Internet Explorer in this regard.

So, this might be an issue we should fix at the cookie level.

Attached is a patch that extends the octal escape sequences already implemented by the SimpleCookie class to include semi-colons and commas. This fixes cookie handling for Safari, Internet Explorer and Chrome. The problem is backwards compatibility - there is a very small chance of some server-side bugs caused by this, but a significantly higher chance of some client-side javascript bugs, if commas or semi-colons are being stored in cookie values. If the javascript code already fully implements the unquoting required by SimpleCookie, it will have no problems, but that seems pretty unlikely.

by Luke Plant, 14 years ago

Attachment: 12470-cookie-fix.diff added

Patch that fixes this at the level of cookie handling

by Luke Plant, 14 years ago

Attachment: 12470-cookie-fix-2.diff added

Added release notes.

by Tobias McNulty, 14 years ago

Attachment: 12670_messages_base64.diff added

base64-encode message payload in CookieStorage to escape special characters and reliably predict cookie size

comment:6 by Luke Plant, 14 years ago

Resolution: fixed
Status: newclosed

(In [12282]) Fixed #12470 - django.contrib.messages CookieStorage failing silently in safari when comma is used in message

This issue was fixed by changing the underlying cookie storage mechanism.

This will fix other bugs with cookies for Internet Explorer and Safari, but
could also cause backwards incompatibilities with existing javascript that
may parse cookie values that contain commas or semi-colons, and, very
rarely, with existing cookie values.

comment:7 by Luke Plant, 14 years ago

(In [12283]) [1.1.X] Fixed #12470 - encoding of comma and semi-colon in cookies.

This is a backport of r12282 from trunk.

The original bug was about CookieStorage, which does not exist in 1.1.X,
but the fix involved the underlying cookie storage.

The change fixes other bugs with cookies for Internet Explorer and Safari,
hence it is backported. It could, however, also cause backwards
incompatibilities with existing javascript that may parse cookie values that
contain commas or semi-colons, and, very rarely, with existing cookie values
manipulated server side.

comment:8 by Michael Malone, 14 years ago

Cc: Michael Malone added
Note: See TracTickets for help on using tickets.
Back to Top