408096 - & not escaped as & in href when creating plaintext url html

Status: RESOLVED FIXED

Description

[Magnus Melin]

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.)

Comment 1

[Magnus Melin]

Created attachment 292830 [details] [diff] [review]
proposed fix

Comment 2

[Ben Bucksch (:BenB)]

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.

Comment 3

[Ben Bucksch (:BenB)]

Created attachment 293397 [details] [diff] [review]
Possible fix, v2, untested

Magnus, this patch implements the above mentioned approach.
It is dry-coded, I have not even compiled it.

Comment 4

[Ben Bucksch (:BenB)]

Ops, in line |aStringToAppendTo.AppendLiteral("&");|, "&amp" needs to be """ instead.

Comment 5

[Magnus Melin]

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;)

Comment 6

[Magnus Melin]

Ben, so did you want me to use the escape function even if & is the only thing we need to care about? 

Comment 7

[Magnus Melin]

Created attachment 295822 [details] [diff] [review]
proposed fix, v3 (v1 with a clarification)

Comment 8

[Magnus Melin]

And a . on the end of the comment, not a comma ;)

Comment 9

[Magnus Melin]

Ben: ping on the r?

Comment 10

[Magnus Melin]

Created attachment 311884 [details] [diff] [review]
proposed fix, v4

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.

Comment 11

[Ben Bucksch (:BenB)]

Comment on attachment 311884 [details] [diff] [review]
proposed fix, v4

r=benb, thanks!

Comment 12

[Ben Bucksch (:BenB)]

(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.)

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 311884 [details] [diff] [review]
proposed fix, v4

+    case '"':
+      if (inAttribute)
+      {
+        aStringToAppendTo.AppendLiteral("&");


you mean " instead o & right?

Comment 14

[Magnus Melin]

Created attachment 314393 [details] [diff] [review]
proposed fix, v5 (for checkin)

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 &

Comment 15

[Phil Ringnalda (:philor)]

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

[Damon Sicore (:damons)]

Need a test case here.  Approval pending test.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 314393 [details] [diff] [review]
proposed fix, v5 (for checkin)

a1.9=beltzner

Comment 18

[Magnus Melin]

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