558253 - Proxy Auto Config (PAC) Extended functions for multi-resolution and ipv6

Status: NEW

(Core :: Networking, enhancement, P3)

Description

[Aron Rubin]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 GTBDFff GTB7.0
Build Identifier: 

There are a couple of problems with the proxy auto config (PAC) API. It does not handle ipv6 and the resolve calls return only the first address. Personally I do not care about ipv6 but the single address is a real problem when it comes to myIpAddress. Virtual adapters/interfaces have addresses too so an address is chosen at random and returned. Microsoft created an extended API for PAC and I see no reason not to adopt it. I have made the necessary changes to nsProxyAutoConfig.js. See the following:
http://blogs.msdn.com/wndp/articles/IPV6_PAC_Extensions_v0_9.aspx
http://code.google.com/p/chromium/issues/detail?id=25407


Reproducible: Always




--- nsProxyAutoConfig.js	2010-04-05 15:20:38 -0400
+++ nsProxyAutoConfig.js	2010-04-08 23:20:31 -0400
@@ -81,6 +81,9 @@ nsProxyAutoConfig.prototype = {
         this._sandBox.importFunction(myIpAddress);
         this._sandBox.importFunction(dnsResolve);
         this._sandBox.importFunction(proxyAlert, "alert");
+        // extended predefined functions
+        this._sandBox.importFunction(myIpAddressEx);
+        this._sandBox.importFunction(dnsResolveEx);
 
         // evaluate loaded js file
         Components.utils.evalInSandbox(pacText, this._sandBox);
@@ -136,6 +139,44 @@ function dnsResolve(host) {
     }
 }
 
+function string_list_from_resolve_list(reslist) {
+    var addrs = [];
+    if( !reslist )
+        return "";
+    while( reslist.hasMore() )
+        addrs.push( reslist.getNextAddrAsString() );
+    return( addrs.join( ";" ) );
+}
+
+// extended wrapper for getting local IP address called by PAC file
+function myIpAddressEx() {
+    try {
+        var reslist = dns.resolve(dns.myHostName, 0);
+        var strlist = string_list_from_resolve_list( reslist );
+        if( strlist != "" )
+            return strlist;
+        else
+            return '127.0.0.1';
+    } catch (e) {
+        return '127.0.0.1';
+    }
+}
+
+// extended wrapper for resolving hostnames called by PAC file
+function dnsResolveEx(host) {
+    host = XPCSafeJSObjectWrapper(host);
+    try {
+        var reslist = dns.resolve(host, 0);
+        var strlist = string_list_from_resolve_list( reslist );
+        if( strlist != "" )
+            return strlist;
+        else
+            return null;
+    } catch (e) {
+        return null;
+    }
+}
+
 var pacModule = new Object();
 
 pacModule.registerSelf =

Comment 1

[Aron Rubin]

Attachment: Patch to add myIpAddressEx and dnsResolveEx

Comment 2

[Kevin Brosnan]

Ask for review from one of the Network module owners, http://www.mozilla.org/about/owners.html . 

https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch#Getting_Reviews

Comment 3

[Aron Rubin]

It would be good to get both a high level reaction as well as a
consideration of the patch itself. For example is this an unreasonable standard or does the implementation need to include ipv6 before accepted?

Comment 4

[Kevin Brosnan]

Set the feedback review request on the patch then.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Sorry for the lag here; I was on leave for a while, and am finally digging through my requests.

The spec seems ok to me at first glance, though this is not exactly my area of expertise.  Including ipv6 would be good if it's easy, but we can probably take it without as a first cut, I think.    Unless we expect compat issues due to PACs assuming ipv6 stuff will be in these functions' return values?

The attached patch doesn't implement the spec, though: the failure case return values are "127.0.0.1" and null instead of "", right?  This is a difference between these functions and the ones without "Ex" that's pointed out in the Chromium bug.

Comment 7

[Niki W. Waibel]

as far as i understand, all _non_ *Ex() functions shall report / work with IPv4 addresses only. currently FFs' myIpAddress() may report ::1 or other IPv6 addresses, even if a proper IPv4 address is assigned to a NIC, depending on the OS configuration (eq /etc/hosts, /etc/nsswitch.conf on unix).

the *Ex() functions, which shall be available within FindProxyforURLEx(url,host), may report / work with IPv6 and IPv4 addresses. myIpAddressEx() is very much required to get a semi-colon separated list of all IP (4 and 6) addresses of the local host. as of my knowledge this is not possible with the old FindProxyforURL(url,host) api.

http://blogs.msdn.com/b/wndp/archive/2006/07/13/ipv6-pac-extensions-v0-9.aspx

please let me emphasize again that IPv6 is a big topic today and workarounds like turning off IPv6 (ie via network.dns.disableIPv6) are not anymore feasable.

above patch may be a good start -- it would even fit my personal requirements -- but i feel simply adding the *Ex() functions for use within FindProxyforURL(url,host) is not correct.

Comment 8

[Mike Kaply [:mkaply]]

The proxy services was completely rewritten in bug 769764 and moved from JavaScript to C++. This was for Firefox 18.

That means that this patch is no longer valid.

I'm adding Patrick McManus to this bug. Maybe since he now has such detailed knowledge of the PAC service from the rewrite, it might be possible to do a patch for this.

Comment 10

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 11

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 12

[arthur200126]

The updated spec is available at https://docs.microsoft.com/en-us/windows/win32/winhttp/ipv6-aware-proxy-helper-api-definitions.

Comment 13

[arthur200126]

So uh... I am looking into fixing this issue, but being new to the code base I have a few questions:

1. It looks like dnsResolve is not doing anything to filter out IPv6 addresses. The upstream nsDNSService (I assume) is indeed capable of giving IPv6 back, and judging from the code dnsResolve might just give an IPv6 address sometime. The thing is I don't really have IPv6 here... Could anyone throw a line of alert(dnsResolve('some.ipv6-only.domain')) in their and confirm the hypothesis for me? [The resolver class is going to walk the whole GetNextAddr chain and filter by the raw.family thing. The dnsResolveEx spec wants us to return a list anyways so we will slways need to change that.]
2. Same question as (1) goes for the myIpAddress. Well, the PACResolver-dependent first step is said to be v6-safe this time, so I guess I was right about the v6 possibility? The myIPAddressEx spec says it wants all unicast addresses for each interface too, so that's a new thing for this module to touch on. Will I find anything useful if I digged into the RTC Network handling for interfaces? (I searched DXR for "eth0".)