223681 - [mozTXTToHTMLConv] fails for links with ipv6 addresses

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

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

I got bugmail about an IPv6 bug, it contained an address like this:
http://[2001:200:0:8002:203:47ff:fea5:3085]/

mailnews turned that into a link pointing to:
http://[2001:200/

the link should've pointed to the entire ip address.

Comment 1

[Ben Bucksch (:BenB)]

I'm not sure, but I suspect the problem to be in Necko. IIRC, the converter
should make the link text identical to the URL found. So, I guess the difference
comes from the URL parser stripping a part. Somebody would have to confirm that
by adding printf statements or using a debugger (I personally don't care enough).

Comment 2

[Ben Bucksch (:BenB)]

Actually, the last ] is not made part of the URL, so that's probably why Necko
fails (assuming it does).

However, ] and [ are disallowed in URLs, IIRC, per RFC2396. So, is this a valid
URL in the first place? If so who says that and how does fit with RFC2396?

Comment 3

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

http://www.faqs.org/rfcs/rfc2732.html

"   This document updates the generic syntax for Uniform Resource
   Identifiers defined in RFC 2396 [URL].  It defines a syntax for IPv6
   addresses and allows the use of "[" and "]" within a URI explicitly
   for this reserved purpose."

Comment 4

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

and yes, a missing ] would cause this:
js> uri = ios.newURI("http://[200:1:2:3", "", null);
[xpconnect wrapped nsIURI]
js> uri.spec
http://[200:1/
js> uri = ios.newURI("http://[200:1:2:3]", "", null);
[xpconnect wrapped nsIURI]
js> uri.spec
http://[200:1:2:3]/

Comment 5

[Ben Bucksch (:BenB)]

> http://www.faqs.org/rfcs/rfc2732.html

OK, thanks, that explains it.

Is there a place where I can see which RFCs update a certain RFC?

> and yes, a missing ] would cause this

Yes, not Necko's fault.


So, the converter needs an update. Either we allow [] only for this purpose, as
the RFC says, but that could need a chunk of code, or we allow [] generally,
slightly against the RFC.

Comment 6

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

> Is there a place where I can see which RFCs update a certain RFC?

sorry... don't know

Comment 7

[John G. Myers]

http://www.ietf.org/iesg/1rfc_index.txt contains information about which RFCs
update other RFCs, but it is incomplete.  I have sent email to the RFC editor
asking for this particular reference to be added to the RFC index.

Comment 8

[John G. Myers]

So, this bug should be reassigned to Necko?  Or should a dependent bug be filed?

Comment 9

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

john: as comment 5 says "Yes, not Necko's fault.", necko would be the wrong
component for this bug

Comment 10

[Lorenzo Colitti]

Where would this change have to be made?

A simple regex match like \[A-Fa-f0-9:\]{0,40} should catch most cases...

Comment 11

[Lorenzo Colitti]

Oops, I thought it was JS code, not C++. Maybe PR_StringToNetAddr could be used
instead...

Comment 12

[Mike Cowperthwaite]

Moving a bunch of mozTXTToHTMLConv bugs into Core / MailNews Backend to live 
with their bretheren.

Comment 13

[(not reading, please use [e-mail] instead)]

sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.

Comment 16

[Ludovic Hirlimann [:Usul]]

Probably a bit late for TB12, but this year will see the launch of ipv6 (in June) Wondering if we should fix this by then.

Comment 17

[Mark Banner (:standard8)]

Not tracking for 12, though we would like to see this fixed.

Comment 18

[Magnus Melin [:mkmelin]]

Taking. 

BenB: I have a vague recollection you have some mail where most url variants are shown, kind of a manual test? That could be useful for creating an automated test where most versions would be checked.

Comment 19

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Allow IPv6 addresses, add some tests.

Comment 20

[Ben Bucksch (:BenB)]

Magnus,

thanks for the nice patch. I'm mostly OK with it.

2 comments:
- Please avoid making changes that are not needed for the fix, e.g. the "else return false;" and the spaces. These things are there quite intentionally.
- Minor: Please make a comment after "seenOpeningParenthesis" // there is a ( in the URL
  ditto for [ , to make this more readable for non-English speakers like me.

Apart from that, r=BenB
Thanks!

Comment 21

[Magnus Melin [:mkmelin]]

Attachment: proposed fix v2

Adding the comment.

Well, no else after return is a mozilla coding style rule, i just took the opportunity to fix it since i was touching code next by - i do think overall readability of the method is important.

Re the 3-7 spaces before parenthesis i don't know why they were there, but if it's supposed to be some kind of styling its different from rest of the code as well as code in general.

Comment 22

[Ben Bucksch (:BenB)]

> i do think overall readability of the method is important.

I agree, and I think if (foo) return 1; else return 2; is more readable.
You still haven't removed this change. Please do.

Comment 23

[Magnus Melin [:mkmelin]]

Ben: https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide item #1
"Don't put an else right after a return. Delete the else, it's unnecessary and increases indentation level."

(Yes there are other places where it should be fixed too, but this happened to be in close by, i'm not so keen on making a style cleanup patch for the whole file.)

Comment 24

[David :Bienvenu]

Magnus is right; the style rules are quite clear on that.

Comment 25

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

Comment on attachment 623411 [details] [diff] [review]
proposed fix v2

sorry for the delay!

Comment 26

[Ben Bucksch (:BenB)]

Attachment: General URL testcases. Not tests for this bug, but regression tests.

Magnus, in comment 18, you asked for my old testcases. Sorry for the delay. Attached here, as RFC822 message file. I don't expect any problems, but would be nice to 1) check them before commit and 2) add them to the unit test that you added. Thanks for the unit test, BTW!

Comment 27

[Magnus Melin [:mkmelin]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/58227b467522

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/58227b467522