Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #1164 (closed defect: fixed)

Opened 11 years ago

Last modified 9 years ago

turbogears.flash() doesn't work if string contains a comma

Reported by: snej Owned by: jorge.vargas
Priority: low Milestone: 1.5
Component: TurboGears Version: 1.0b1
Severity: trivial Keywords:
Cc:

Description

The turbogears.flash() function doesn't work properly (usually has no visible effect) if the message string parameter contains a comma (",") character. I just spent about 20 minutes tearing out my hair trying to figure out why the message wouldn't show up.

I believe this happens because the comma is a delimiter in HTTP cookie syntax. TG would need to strip or escape comma characters when setting cookies to avoid this problem. Alternatively, it could store the flash message in the session dictionary instead of using a raw cookie for it.

Attachments

test.patch Download (1.5 KB) - added by Joost 11 years ago.
test

Change History

Changed 11 years ago by Joost

test

comment:1 follow-up: ↓ 5 Changed 11 years ago by Joost

I can't reproduce this problem in quickstarted project nor does the included regression test fail.

comment:2 Changed 11 years ago by jorge.vargas

  • Priority changed from normal to low
  • Owner changed from anonymous to jorge.vargas
  • Severity changed from normal to trivial
  • Milestone set to 1.0.2

this seems something simple to test.

comment:3 Changed 11 years ago by jorge.vargas

using your patch against trunk in r2527 the test passed here as well.

comment:4 Changed 11 years ago by plewis

tests in patch pass for me as well (1.0 branch, r2549)

comment:5 in reply to: ↑ 1 Changed 10 years ago by eaganj

Replying to Joost:

I can't reproduce this problem in quickstarted project nor does the included regression test fail.

This bug just cost me several hours and locks of hair. It appears to only happen in Safari (2.0.4/419.3) on the Mac, but not in Firefox. It is most definitely a real bug (whether in Safari or in TurboGears).

comment:6 Changed 10 years ago by jonypawks

I just tested this as well with [r2858]. The test added by the patch worked fine and the flash message was displayed properly in Safari (2.0.4/419.3) with a quickstarted app.

comment:7 Changed 10 years ago by alberto

  • Milestone changed from 1.0.2 to 1.0.3

comment:8 Changed 10 years ago by faide

  • Milestone changed from 1.0.3 to 1.1

Can someone with Safari confirm if he can display a flash including commas ?

comment:9 Changed 9 years ago by chrisz

This has come up on the mailing list again and I can confirm that this is still a problem with TG 1.0.4.4 and the current Safari 3.1 on Windows.

I think the problem is that flash messages are stored as raw text strings in a cookie, but cookie values should actually not contain commas, semicolons or even whitespace ( http://wp.netscape.com/newsref/std/cookie_spec.html). The unit test won't show the problem because the CherryPy? and httptools client handle commas gracefully, as well as other browsers.

To fix this problem in TG 1.0.x I suggest either using urrlib.quote/unquote for escaping the flash message, or using a special function that only escapes commas, semicolons and whitespace.

And TG 1.1 and up should get a more sophisticated flash function using the CherryPy? session anyway.

comment:10 Changed 9 years ago by chrisz

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

Fixed in r4343 now, using urllib.quote/unquote, but only encoding the few troubled chars with quote, not all.

Btw, another reason why the attached unit test never detected any problem was that it did not do a redirect, so no cookie was involved at all. I have checked in a modified test with redirect that also checks the cookie directly for illegal chars.

Note: See TracTickets for help on using tickets.