Ticket #1164 (closed defect: fixed)
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
(1.5 KB) -
added by Joost 5 years ago.
- test
Change History
Changed 5 years ago by Joost
-
attachment
test.patch
added
comment:1 follow-up: ↓ 5 Changed 5 years ago by Joost
I can't reproduce this problem in quickstarted project nor does the included regression test fail.
comment:2 Changed 5 years ago by jorge.vargas
- Owner changed from anonymous to jorge.vargas
- Priority changed from normal to low
- Severity changed from normal to trivial
- Milestone set to 1.0.2
this seems something simple to test.
comment:3 Changed 5 years ago by jorge.vargas
using your patch against trunk in r2527 the test passed here as well.
comment:5 in reply to: ↑ 1 Changed 5 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 5 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:8 Changed 5 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 4 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 4 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.
test