27930 - URL parser should support RFC 2732 - IPv6 address literals

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P3)

Description

[John G. Myers]

The URL parser should support RFC 2732 syntax.  For example, it should handle

http://[207.200.73.41]/

Comment 1

[John G. Myers]

Oops, bracketed IPv4 addresses are not legal per RFC 2732.

This bug then depends on IPv6 support.  Reassigning to myself until bug 23811 is 
resolved.

Comment 2

[John G. Myers]

Attachment: proposed fix

Comment 3

[John G. Myers]

Assign to module owner for review and checkin.
This can be checked in before 23811, but cannot be tested until 23811 is fixed.

Comment 4

[Andreas Otte]

The changes look good to me from a first peek at them.

Comment 5

[Warren Harris]

Moving what's not done for M15 to M16.

Comment 6

[Wan-Teh Chang]

Someone pointed out to me that there is an alternative
syntax for specifying literal IPv6 addresses in URL's:
http://www.ics.uci.edu/pub/ietf/uri/draft-masinter-url-ipv6-02.txt

Quoted from this Internet-Draft:
   The syntax is best described as a transformation of the
   normal IPv6 syntax as defined in section 2.2 of [RFC2373];
   starting with such an address:

     1) replace every colon ":" with a "-"
     2) append ".ipv6" to the end

   Thus, an HTTP service available at port 70 of IPv6 address
   "ABCD:EF01::2345:10.9.8.7" could be written as
       
        http://ABCD-EF01--2345-10.9.8.7.ipv6:70/

Comment 7

[Wan-Teh Chang]

A useful URL on URI: http://www.ics.uci.edu/pub/ietf/uri/.

Comment 8

[Andreas Otte]

This alternativ syntax is really nice since we do not have to change the
urlparser for it.

Comment 9

[John G. Myers]

The alternative syntax is a year-old internet draft.  RFC 2732, which is 

implemented by the patch attached to this bug, is a Proposed Standard.

Comment 10

[John G. Myers]

Working on an updated patch

Comment 11

[John G. Myers]

Attachment: updated patch

Comment 12

[John G. Myers]

Reassign for review.

Comment 13

[John G. Myers]

I'm concerned that my patch doesn't include any changes to nsNoAuthURLParser. 
How can I construct a test case that excercises this parser?

Comment 14

[Andreas Otte]

Use a file url ...

Comment 15

[Andreas Otte]

Ahemmm ... of course as the name says there is not authority -> no host in these 
cases. So you don't have to be concerned.

Comment 16

[Wan-Teh Chang]

Regarding the alternative syntax, Larry Masinter <[e-mail]>
says in netscape.public.mozilla.netlib:
    This is obsolete; after much discussion, it was clear that
    this approach wasn't going to work for a lot of reasons.
    Don't do this. The draft should have expired by now.

Comment 17

[Wan-Teh Chang]

I reviewed jgmyers's latest patch (id=7929).
I have the following questions.
1. In this code fragment:
    case '[':
        if (brk == i_Spec) {
            rv = ParseAtHost(i_Spec, o_Host, o_Port, o_Path);
            if (rv != NS_ERROR_MALFORMED_URI) return rv;

            // Try something else
            CRTFREEIF(*o_Host);
            *o_Port = -1;
        }
        rv = ParseAtPath(i_Spec, o_Path);
        return rv;
  is it really necessary to free *o_Host and
  reset *o_Port when ParseAtHost fails with
  NS_ERROR_MALFORMED_URI?  What about *o_Path?

  It would be better to ensure that ParseAtHost
  does not allocate *o_Host and *o_Path when it
  fails.

2. In ParseAtHost, the new switch statement is
   missing the case of '#', which is a member of
   'delimiters'.
                switch (*fwdPtr)
                {
                case '\0': // everything is a host
                    return NS_OK;
                case '/' :
                case '?' :
                    rv = ParseAtPath(fwdPtr, o_Path);
                    return rv;
                case ':' :
                    rv = ParseAtPort(fwdPtr+1, o_Port, o_Path);
                    return rv;
                default:
                    NS_ASSERTION(0, "This just can't be!");
                    break;
                }

Some comments explaining the difference between ESCAPED
and HOSTESCAPED in nsStdURL.h or nsStdURL::AppendString
would be nice.  In particular the fact that HOSTESCAPED
implies ESCAPED is not obvious.

Other than these two questions, I think this patch
is good.  Caveat: I am not familiar with the
Necko code, so my code review is limited to the
code that this patch modifies.

By the way, I found that in nsAuthURLParser.cpp
and nsStdURLParser.cpp, the local variable 'fwdPtr'
casts away the const of 'i_Spec'.  Is that
intentional?

Comment 18

[John G. Myers]

Attachment: updated patch per review comments

Comment 19

[John G. Myers]

Review replies:

1. This code follows the convention established by the "Try something else" code 
following calls to ParseAtPreHost().  o_Path need not be tested as it is the 
last argument--if it is filled in, then the URI is not malformed.  I agree it 
would be better to ensure that the ParseAt* routines do not allocate output 
parameters on failure, but do not want this patch to change the interface 
contract.

2. Added the '#' case in the updated patch.

In the updated patch, the fwdPtr local added by the patch is now const.  The 
patch does not change existing fwdPtr declarations.

In the updated patch, there are now comments added to nsStdURL.h.  I would not 
say that HOSTESCAPED implies ESCAPED.  HOSTESCAPED is just a different set of 
escaping rules.  It is similar to but distinct from ESCAPED.

Comment 20

[Wan-Teh Chang]

Attachment id=8272 has Windows line endings.
If you want to apply the patch on Unix, be
sure to convert the line endings first.

Gagan, are you planning to check in this
patch before 5/16?

Comment 21

[leger]

Putting on [nsbeta2+][5/16] radar.  This is a feature MUST complete work by 
05/16 or we may pull this feature for PR2.

Comment 22

[John G. Myers]

Attachment: Updated patch (resolve conflicts)

Comment 23

[Andreas Otte]

Hi John, don't forget to look at nsNoAuthURLParser.cpp. The hostname sneaked
back in over the weekend, you have to look there too for IP6-Adresses.

Comment 24

[John G. Myers]

If IPv6 address literals could contain '/' characters, that would be an issue.  
As they currently cannot, it is not an issue.

Comment 25

[John G. Myers]

r=gagan, per email sent 5/10.

Comment 26

[Gagan]

I will check this in tonite. 

Comment 27

[Gagan]

checked in.

Comment 28

[Andreas Otte]

I have reopened this bug because the host was back in nsNoAuthURLParser before
the IP6 stuff was checked in. Currently we do not watch for IP6 adresses there
and consequently sometimes add [] around a hostname that looks like an IP6
address.

Comment 29

[Andreas Otte]

Attachment: proposed patch, warning: contains also fixes for bug 39340

Comment 30

[John G. Myers]

Could you provide a test case?

Comment 31

[Andreas Otte]

You can always use urltest with a file url like file://[::ffff:207.200.73.41]/

Comment 32

[Andreas Otte]

Check it on linux/unix.

Comment 33

[leger]

Putting on [nsbeta2-] radar.  Missed the feature train. Please set to MFuture.

Comment 34

[Andreas Otte]

leger: why did it miss the feature train? IPv6 suport is in, it is just missing
at one place in the code. nsStdURL now expects correct IPv6 handling which is
not done inside nsNoAuthURLParser. This is no missing feature, just a plain bug.

Comment 35

[Wan-Teh Chang]

Andreas: since you are not a Netscape employee,
my understanding is that your checkins only
need to be approved by the module owner and
brendan or waterson.

Comment 36

[Andreas Otte]

fix checked in

Comment 37

[benc]

Relieving tever of IPv6 related QA.

Comment 38

[benc]

+verifyme

I won't be looking at IPv6 until marketing tells me it's a feature

Comment 39

[benc]

off netscape radar.

Comment 40

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old