Closed
Bug 408096
Opened 17 years ago
Closed 16 years ago
& not escaped as & in href when creating plaintext url html
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9
People
(Reporter: mkmelin, Assigned: mkmelin)
Details
Attachments
(1 file, 4 obsolete files)
7.01 KB,
patch
|
mkmelin
:
review+
mkmelin
:
superreview+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
STR: - Compose a new mail in thunderbird, make sure you are in html composition mode - paste an url with &, e.g. http://www.example.com/?test=true&passed=uncertain&test2=ok - make sure we won't convert to plain text by e.g. entering also some text and making it really large - send to yourself, make sure to include a html mode mail (if tb asks you) Observe the received mail which should be in html format. In the actual href="..." the & is not escaped as & it should be. (In the link text it's escaped properly already.)
Assignee | ||
Comment 1•17 years ago
|
||
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #292830 -
Flags: superreview?(bienvenu)
Attachment #292830 -
Flags: review?(ben.bucksch)
Comment 2•17 years ago
|
||
Comment on attachment 292830 [details] [diff] [review] proposed fix Ops, yes, this needs to be escaped. <http://www.w3.org/TR/html4/charset.html#h-5.3.2> r- on the patch, though. It only escapes this one character, but > is just as problematic, and " is even more problematic. You should use the function EscapeStr (line 90); see also comment at its top. Unfortunately, it does not escape " , because that's not necessary outside attribute values, and I'd prefer to keep it that way. I'll attach a new patch which does that.
Attachment #292830 -
Flags: review?(ben.bucksch) → review-
Comment 3•17 years ago
|
||
Magnus, this patch implements the above mentioned approach. It is dry-coded, I have not even compiled it.
Attachment #292830 -
Attachment is obsolete: true
Attachment #292830 -
Flags: superreview?(bienvenu)
Comment 4•17 years ago
|
||
Ops, in line |aStringToAppendTo.AppendLiteral("&");|, "&" needs to be """ instead.
Assignee | ||
Comment 5•17 years ago
|
||
Hm, but <, > and " aren't valid inside an url, so it we'll never come there. I tried an url with ' earlier, and that's fine too as it's %-escaped at that point. Bc of the mIOService->NewURI earlier I suppose. Should probably leave a comment about it in the code though;)
Assignee | ||
Comment 6•17 years ago
|
||
Ben, so did you want me to use the escape function even if & is the only thing we need to care about?
Assignee | ||
Comment 7•17 years ago
|
||
Attachment #293397 -
Attachment is obsolete: true
Attachment #295822 -
Flags: review?(ben.bucksch)
Assignee | ||
Comment 8•17 years ago
|
||
And a . on the end of the comment, not a comma ;)
Assignee | ||
Comment 9•16 years ago
|
||
Ben: ping on the r?
Assignee | ||
Comment 10•16 years ago
|
||
This is an updated (compiling) version of Ben's patch (v2). Changes: CheckURLAndCreateHTML takes a const txtURL as argument, so we can't modify that directly, and using PRBools instead of bools.
Attachment #295822 -
Attachment is obsolete: true
Attachment #311884 -
Flags: review?(ben.bucksch)
Attachment #295822 -
Flags: review?(ben.bucksch)
Comment 11•16 years ago
|
||
Comment on attachment 311884 [details] [diff] [review] proposed fix, v4 r=benb, thanks!
Attachment #311884 -
Flags: review?(ben.bucksch) → review+
Comment 12•16 years ago
|
||
(I'm not sure whether I'm formally allowed to review this code, though. I'm neither mail nor netwerk module peer. In previous patches, I think this class has been treated as mail code.)
Assignee | ||
Updated•16 years ago
|
Attachment #311884 -
Flags: superreview?(cbiesinger)
Comment 13•16 years ago
|
||
Comment on attachment 311884 [details] [diff] [review] proposed fix, v4 + case '"': + if (inAttribute) + { + aStringToAppendTo.AppendLiteral("&"); you mean " instead o & right?
Attachment #311884 -
Flags: superreview?(cbiesinger) → superreview+
Assignee | ||
Comment 14•16 years ago
|
||
Absolutely! Carrying over the r=benb, sr=cbiesinger, asking approval. I'm not sure if firefox really exercise this code anywhere (and is subject to approval), but it's a safe fix. Just makes auto recognized urls we send out in html format standards compliant by escaping the & as &
Attachment #311884 -
Attachment is obsolete: true
Attachment #314393 -
Flags: superreview+
Attachment #314393 -
Flags: review+
Attachment #314393 -
Flags: approval1.9?
Comment 15•16 years ago
|
||
Pretty sure the closest Fx gets to it (barring builds with either Gopher or Finger enabled) is to call FindURLInPlaintext from spellcheck code, so as long as http://foo.com/?bar=baz&quux=fxhq and http://foo.com/?bar=baz&quux=fxhq both remain not-spellchecked, you should be fine in Fx terms.
Comment 16•16 years ago
|
||
Need a test case here. Approval pending test.
Comment 17•16 years ago
|
||
Comment on attachment 314393 [details] [diff] [review] proposed fix, v5 (for checkin) a1.9=beltzner
Attachment #314393 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 18•16 years ago
|
||
Checking in netwerk/streamconv/converters/mozTXTToHTMLConv.cpp; /cvsroot/mozilla/netwerk/streamconv/converters/mozTXTToHTMLConv.cpp,v <-- mozTXTToHTMLConv.cpp new revision: 1.91; previous revision: 1.90 done Checking in netwerk/streamconv/converters/mozTXTToHTMLConv.h; /cvsroot/mozilla/netwerk/streamconv/converters/mozTXTToHTMLConv.h,v <-- mozTXTToHTMLConv.h new revision: 1.22; previous revision: 1.21 done ->FIXED
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•