Closed Bug 960014 Opened 10 years ago Closed 10 years ago

Make nsStandardURL::SetHost less magical around IPv6

Categories

(Core :: Networking, defect)

Product:
Core
Component:
Networking
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla32

People

(Reporter: annevk, Assigned: valentin)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 10 obsolete files)

Make nsStandardURL::SetHost less magical around IPv6
10.63 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
Escape IPv6 address in URL::GetHostname
2.41 KB, patch
Details | Diff | Splinter Review 
In Gecko url.hostname = "2001::1" works. Typically a host parser would require [ and ] there but we have some magic that inserts them. And when serializing (in nsStandardURL.h) we have some magic that removes them.

https://www.w3.org/Bugs/Public/show_bug.cgi?id=11587 is a bug filed against http://url.spec.whatwg.org/ to standardize this behavior which Internet Explorer also exhibits.

This bug is a request to instead change Gecko as the behavior is rather quirky.
Blocks: url
Assignee: nobody → valentin.gosu
If fixed, what would url.hostname = "2001::1" do? And afterwards, what would url.hostname return? (Chromium seems to return "")
Per http://url.spec.whatwg.org/#hostname-state that would result in URL's host not being changed so url.hostname would return the value it returned before it was set.
Comment on attachment 8414101 [details] [diff] [review]
Make nsStandardURL::SetHost less magical around IPv6

Requesting preliminary feedback.
I still need to add testing for these usecases and push to try.
Attachment #8414101 - Flags: feedback?(mcmanus)
I'm for changing the spec. Is the spec bug already WONTFIXed?
Comment on attachment 8414101 [details] [diff] [review]
Make nsStandardURL::SetHost less magical around IPv6

Review of attachment 8414101 [details] [diff] [review]:
-----------------------------------------------------------------

if anne wants to move us from being ie compat to being chrome compat in this case I'm ok with it
Attachment #8414101 - Flags: feedback?(mcmanus) → feedback+
Attachment #8414101 - Attachment is obsolete: true
Anne, can you confirm that the following operations should succeed?

var url = new URL("http://localhost/");
url.host = "[2001::1]"; // does not work in Chrome
url.host = "[2001::1]:20"; // does not work in Chrome

Thanks!
Flags: needinfo?(annevk)
Attachment #8416290 - Flags: review?(mcmanus)
Per the specification they should, yes. It also makes sense. 

In IE10 however it also does not quite work. Printing url.host after setting it per comment 8:

* localhost:80
* [2001::1:80
* 2001::1:20
Flags: needinfo?(annevk)
This fixes the failing tests: https://tbpl.mozilla.org/?tree=Try&rev=25fe2bec619f

Honza, as a security/manager peer, could you take a look at this?

Tests passing with this patch: https://tbpl.mozilla.org/?tree=Try&rev=859cbaef6efa
Attachment #8416524 - Flags: review?(honzab.moz)
Comment on attachment 8416290 [details] [diff] [review]
Make nsStandardURL::SetHost less magical around IPv6

Review of attachment 8416290 [details] [diff] [review]:
-----------------------------------------------------------------

honza is probably a better reviewer for this part too.. honza see comment 6 - I'm fine with the stated goal of the bug
Attachment #8416290 - Flags: review?(mcmanus) → review?(honzab.moz)
I'll do this next week.
Thanks!
Review in draft, I'll finish tomorrow (Tuesday)
Comment on attachment 8416524 [details] [diff] [review]
Fix uses of url.host in test_URI.js and nsSiteSecurityService.cpp

Review of attachment 8416524 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/boot/src/nsSiteSecurityService.cpp
@@ +251,5 @@
>    nsresult rv = GetHost(aSourceURI, host);
>    NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsAutoCString hostIP(host);
> +  if (host[0]=='[' && host[host.Length()-1]==']') {

err... what if host is an empty string?  don't rely on GetHost return a non-empty string

spaces around ==

maybe rather use functions from http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsStringAPI.h#1374

@@ +252,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsAutoCString hostIP(host);
> +  if (host[0]=='[' && host[host.Length()-1]==']') {
> +    hostIP = Substring(host, 1, host.Length() -2);

s/-2/- 2/

@@ +462,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  nsAutoCString hostIP(host);
> +  if (host[0]=='[' && host[host.Length()-1]==']') {
> +    hostIP = Substring(host, 1, host.Length() -2);

same here
Attachment #8416524 - Flags: review?(honzab.moz) → review-
Comment on attachment 8416290 [details] [diff] [review]
Make nsStandardURL::SetHost less magical around IPv6

Review of attachment 8416290 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/src/nsStandardURL.cpp
@@ +435,5 @@
> +    if (hasBrackets) {
> +        return net_IsValidIPv6Addr(host+1, length-2);
> +    }
> +
> +    if (host[0] == '[' || host[length-1] == ']') {

maybe have bool openBracket = host[0] == '[' and bool closeBracket and work with them instead

@@ +1443,5 @@
> +    aValue.EndReading(end);
> +    nsACString::const_iterator iter(start);
> +    bool isIPv6 = false;
> +
> +    if (aValue[0] == '[') { // IPv6 address

s/aValue[0]/*start/

@@ +1448,5 @@
> +        isIPv6 = true;
> +        if (!FindCharInReadable(']', iter, end)) {
> +            // the ] character is missing
> +            return NS_ERROR_MALFORMED_URI;
> +        }

comment:

// iter now at the ']' character

@@ +1455,5 @@
> +    FindCharInReadable(':', iter, end);
> +
> +    if (!isIPv6 && iter != end) {
> +        nsACString::const_iterator iter2(iter);
> +        if (FindCharInReadable(':', iter2, end)) {

you must ++iter2 before this line since iter is here at ':' and FindCharInReadable will succeed.

@@ +1465,2 @@
>  
>    nsresult rv = SetHost(Substring(start, iter));

when you are here, please fix the indention according file style (4 sp)

@@ +1517,5 @@
>      InvalidateCache();
>      mHostEncoding = eEncoding_ASCII;
>  
> +    if (!ValidIPv6orHostname(host)) {
> +        return NS_ERROR_MALFORMED_URI;

do this before InvalidateCache() and mHostEncoding reset.

@@ -1494,5 @@
> -            mPassword.mLen = -1;
> -            mHost.mLen = -1;
> -            mPort = -1;
> -        }
> -        return NS_OK;

why has this been removed?

::: netwerk/test/unit/test_standardurl.js
@@ +168,5 @@
> +  var url = stringToURL("http://example.com");
> +  url.host = "[2001::1]";
> +  do_check_eq(url.host, "[2001::1]");
> +
> +  var url = stringToURL("http://example.com");

you are redeclaring

@@ +172,5 @@
> +  var url = stringToURL("http://example.com");
> +  url.hostPort = "[2001::1]:30";
> +  do_check_eq(url.host, "[2001::1]");
> +  do_check_eq(url.port, 30);
> +  do_check_eq(url.hostPort, "[2001::1]:30");

check also hostPort = 2001:10 (should work)

@@ +186,5 @@
> +  Assert.throws(() => { url.host = "2001[::1]"; }, "bad bracket position");
> +  Assert.throws(() => { url.host = "[]"; }, "empty IPv6 address");
> +  Assert.throws(() => { url.host = "[hello]"; }, "bad IPv6 address");
> +  Assert.throws(() => { url.hostPort = "2001::1"; }, "enclosed in brackets");
> +  Assert.throws(() => { url.hostPort = "[2001::1]30"; }, "missing : after IP");

check also (set hostPort)
[2001:1]
[2001:1]10:20
[2001:1]:10:20
[2001:1
2001]:1



I checked also "2001:1]".  The port string is "1]" and portStr.ToInteger(&rv); silently parses it as 1.  Weird.
Thanks for the review.

(In reply to Honza Bambas (:mayhemer) from comment #16)
> @@ -1494,5 @@
> > -            mPassword.mLen = -1;
> > -            mHost.mLen = -1;
> > -            mPort = -1;
> > -        }
> > -        return NS_OK;
> 
> why has this been removed?

That block is now inaccessible because of Bug 996055 where we return early if the input is an empty string.
Enforces brackets for IPv6 URLs
Removed unreachable _if (!*host)_ block in nsStandardURL::SetHost
SetHostPort fails for empty and non-int strings
Attachment #8416290 - Attachment is obsolete: true
Attachment #8416290 - Flags: review?(honzab.moz)
Attachment #8416524 - Attachment is obsolete: true
Comment on attachment 8418474 [details] [diff] [review]
Make nsStandardURL::SetHost less magical around IPv6

I added the tests you suggested, and fixed a few other issues where we didn't throw an error when parsing the port number.
Attachment #8418474 - Flags: review?(honzab.moz)
Comment on attachment 8418475 [details] [diff] [review]
Fix uses of url.host in test_URI.js and nsSiteSecurityService.cpp

https://tbpl.mozilla.org/?tree=Try&rev=c16cde3c1ff6
Attachment #8418475 - Flags: review?(honzab.moz)
Comment on attachment 8418474 [details] [diff] [review]
Make nsStandardURL::SetHost less magical around IPv6

Review of attachment 8418474 [details] [diff] [review]:
-----------------------------------------------------------------

I'll be so happy to rewrite this all with some more intelligent and safer way of parsing as part of the mutable/immutable/thread-safe URI rewrite..

::: netwerk/base/src/nsStandardURL.cpp
@@ +1445,5 @@
> +    nsACString::const_iterator iter(start);
> +    bool isIPv6 = false;
> +
> +    if (*start == '[') { // IPv6 address
> +        isIPv6 = true;

maybe set this flag after you check the ending ']'

@@ +1452,5 @@
> +            return NS_ERROR_MALFORMED_URI;
> +        }
> +        // iter now at the ']' character
> +    } else {
> +        nsACString::const_iterator iter2(iter);

instead iter2(iter); do iter2(start); since iter == start here (confusing)
Attachment #8418474 - Flags: review?(honzab.moz) → review+
Attachment #8418475 - Flags: review?(honzab.moz) → review+
When looking into the SSS, bad think is that we use the host name (formerly w/o [ ], now with [ ]) to persist permissions.  This can break stuff in the wild.  Not sure how serious issue this is tho.

Boris?
Flags: needinfo?(bzbarsky)
This will only be an issue if someone has permissions stored for a URL that had an IPv6 address as the host, right?
Flags: needinfo?(bzbarsky)
Enforces brackets for IPv6 URLs
Removed unreachable _if (!*host)_ block in nsStandardURL::SetHost
SetHostPort fails for empty and non-int strings
Attachment #8418474 - Attachment is obsolete: true
(In reply to Boris Zbarsky [:bz] from comment #24)
> This will only be an issue if someone has permissions stored for a URL that
> had an IPv6 address as the host, right?

I think so. How could I test for that? ( as in how do I trigger the permissions to be saved? Would accessing a local ipv6 address with a self signed cert be enough? )

Thanks
Flags: needinfo?(bzbarsky)
I don't know offhand...  Maybe try unblocking popups from a local ipv6 address and seeing if that persists across the update?
Flags: needinfo?(bzbarsky)
Here are my latest findings:

It turns out that while the patch fixes the spec issue, it also breaks browsing to http://[::1]/ and that we don't have any tests checking for this.

One solution would be to fix all of the calls to GetHost checking if it's an IPv6 address, and remove the brackets if needed. There are about 100 references in the tree, and this seems a bit messy.

The second solution would be to keep GetHost the way it is, returning unescaped IPv6 addresses, and only to escape it in the DOM Url implementation of GetHostname. It's less of a risk for unexpected behaviour and a much smaller patch.

Please let me know if you guys think there's any point in trying the first approach.
Enforces brackets for IPv6 URLs
Removed unreachable _if (!*host)_ block in nsStandardURL::SetHost
SetHostPort fails for empty and non-int strings
Attachment #8421785 - Attachment is obsolete: true
Attachment #8418475 - Attachment is obsolete: true
Attachment #8425877 - Flags: review?(bugs)
Comment on attachment 8425877 [details] [diff] [review]
Escape IPv6 address in URL::GetHostname


> URL::GetHostname(nsString& aHostname) const
> {
>-  URL_GETTER(aHostname, GetHost);
>+  aHostname.Truncate();
>+  nsAutoCString tmp;
>+  nsresult rv = mURI->GetHost(tmp);
>+  if (NS_SUCCEEDED(rv)) {
>+    if (tmp.FindChar(':')) { // Escape IPv6 address
Could you assert here that first and last chars aren't [ and ]



>+      tmp.Insert('[', 0);
>+      tmp.Append(']');
>+    }
>+    CopyUTF8toUTF16(tmp, aHostname);
>+  }
> }


This needs some tests, and test also host, not only hostname
Attachment #8425877 - Flags: review?(bugs) → review-
Enforces brackets for IPv6 URLs in SetHost
Removed unreachable _if (!*host)_ block in nsStandardURL::SetHost
SetHostPort fails for empty and non-int strings
Attachment #8425874 - Attachment is obsolete: true
Attachment #8425877 - Attachment is obsolete: true
Attachment #8425953 - Attachment is obsolete: true
Comment on attachment 8425980 [details] [diff] [review]
Escape IPv6 address in URL::GetHostname

I've addressed your comments and added some tests.
Attachment #8425980 - Flags: review?(bugs)
Comment on attachment 8425980 [details] [diff] [review]
Escape IPv6 address in URL::GetHostname

>+      MOZ_ASSERT(!tmp.Length() || (tmp[0] !='[' && tmp[tmp.Length()-1] != ']'));
space before and after -


>+    url = new URL("http://localhost/");
>+    url.host = "[2001::1]:30";
>+    is(url.hostname, "[2001::1]", "IPv6 hostname");
>+    is(url.port, 30, "Port");
Could you test that host is sane here.
Attachment #8425980 - Flags: review?(bugs) → review+
Comment on attachment 8425952 [details] [diff] [review]
Make nsStandardURL::SetHost less magical around IPv6

Basically the same patch from before, with some bits removed (not escaping nsStandardURL::GetHost anymore)
Attachment #8425952 - Flags: review?(honzab.moz)
Attachment #8425980 - Attachment is obsolete: true
When do you need the review?  This needs even more careful look from me than the last time..
(In reply to Honza Bambas (:mayhemer) from comment #39)
> When do you need the review?  This needs even more careful look from me than
> the last time..
I don't think there's a sense of urgency for landing this, but if you have any concerns about the patch, I'm willing to address them.
It's basically the same code you r+'ed last time, but without the escaping of nsStandardURL::GetHost.
I think it's much safer this way, since consumers of that function don't need to alter their behaviour.
Comment on attachment 8425952 [details] [diff] [review]
Make nsStandardURL::SetHost less magical around IPv6

Review of attachment 8425952 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/test/unit/test_standardurl.js
@@ +193,5 @@
> +  Assert.throws(() => { url.host = "2001[::1]"; }, "bad bracket position");
> +  Assert.throws(() => { url.host = "[]"; }, "empty IPv6 address");
> +  Assert.throws(() => { url.host = "[hello]"; }, "bad IPv6 address");
> +  Assert.throws(() => { url.host = "[192.168.1.1]"; }, "bad IPv6 address");
> +  Assert.throws(() => { url.hostPort = "2001::1"; }, "enclosed in brackets");

it's actually not enclosed in brackets
Attachment #8425952 - Flags: review?(honzab.moz) → review+
https://hg.mozilla.org/mozilla-central/rev/37df28bdb54a
https://hg.mozilla.org/mozilla-central/rev/849e8fe4e4a4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1022207
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: