Closed Bug 1353510 Opened 8 years ago Closed 7 years ago

Add support for the global PAC functions

Categories

(WebExtensions :: Request Handling, enhancement, P3)

enhancement

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: mattw, Assigned: mixedpuppy)

References

(Blocks 1 open bug)

Details

(Whiteboard: [proxy] triaged)

Attachments

(1 file)

See http://findproxyforurl.com/pac-functions/ for details about PAC functions. For more information about the Proxy API, see the design document: https://docs.google.com/document/d/1W45o5X2bFRPrTaQDFp9IzTJ8njCVfEgyENS7i2owaUI/edit?usp=sharing
These implementations can be used in the WebExtensions Proxy API: https://dxr.mozilla.org/mozilla/source/netwerk/base/src/nsProxyAutoConfig.js
Is nsProxyAutoConfig.js available in FF55+? Can it be imported into nsProxyAutoConfig.js? What about ProxyAutoConfig.cpp?
I have started to write the PAC functions. Once finished, I will pass it Matthew for testing and implementation.
re: DNS nsIProtocolProxyService already has the DNS methods. The resolve() is deprecated and asyncResolve() complicates the PAC dnsResolve(host) function. ProxyAutoConfig.cpp uses C++ for dnsResolve(host) and myIpAddress(). There are other PAC functions also that depend on dnsResolve() (IP based) and they are all synchronous eg: isInNet() isResolvable() myIpAddress() Any suggestions? PS. I have prepared the rest (based on ProxyAutoConfig.cpp with some modifications/updates)
I have written dnsResolve() as asynchronous asyncDnsResolve() however, the function would no longer match standard PAC functions and anyone using it will have to write their code to suit a asynchronous result with callback.
If you look at https://reviewboard.mozilla.org/r/73182/diff/3/, I originally had a partial implementation of the proxy globals - in there you can see I have an implementation of dnsResolve, which if it works, I believe is synchronous. I ended up removing them from that bug because the patch was getting too large, and I'd also really like to have unit tests for the functions.
resolve() has been deprecated since Gecko 18 > This method has been removed in Firefox 18. Use resolveAsync instead ref: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIProtocolProxyService No mention here but I am guessing it is removed and the MDN hasn't been updated. https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIDNSService What I was working on .... ext-proxy.js already imports: ProxyScriptContext.jsm ProxyScriptContext.jsm also imports: XPCOMUtils.defineLazyServiceGetter(this, "ProxyService", "@mozilla.org/network/protocol-proxy-service;1", "nsIProtocolProxyService"); Therefore there is already access to nsIProtocolProxyService I have prepared all PAC functions that don't need dnsresolve() as pure JavaScript code. I have also written the ones that need DNS, as both sync and async functions. These are in plain JS and not prepared as API. The discrepancy on MDN regarding isInNet() has to be decided on since otherwise isInNet()can be done with pure JavaScript but the way Firefox has it with built-in dnsResolve(), requires rewriting it as async https://developer.mozilla.org/en-US/docs/Web/HTTP/Proxy_servers_and_tunneling/Proxy_Auto-Configuration_(PAC)_file#isInNet()
re: isInNet() Firefox ProxyAutoConfig.cpp impemented isInNet() with built-in dnsResolve() Oracle Java also does the same . https://docs.oracle.com/cd/E19575-01/821-0053/adyrv/index.html PAC Functions states: https://findproxyforurl.com/pac-functions/ > isInNet > This function evaluates the IP address of a hostname, and if within a specified subnet returns true. If a hostname is passed the function will resolve the hostname to an IP address. However, the examples show that dnsResolve() have been passed as a separate function which shows isInNet() in itself does not resolve the DNS. > if (isInNet(dnsResolve(host), "172.16.0.0", "255.240.0.0")) A decision has to be made on the proper implementation.
These functions can be added to the scope of the PAC by defining them on |this.sandbox| in ProxyScriptContext.jsm. To see how its done at the browser scope (not WebExtensions): https://dxr.mozilla.org/mozilla/source/netwerk/base/src/nsProxyAutoConfig.js#84
Changing to P2 because some of these functions can be implemented by addon authors themselves in their addon's FindProxyForURL() module. Copy Javascript implementations from https://dxr.mozilla.org/mozilla/source/netwerk/base/src/nsProxyAutoConfig.js to your addon. Ones related to dns lookups cannot be implemented that way. @erosman, concerning dnsResolve(): bug 887995 added AsyncResolve2(), which does asynchronous DNS resolution, but according to John-Galt the entire FindProxyForURL() is synchronous and therefore can't do asynchronous DNS resolution in any way.
Priority: -- → P3
Priority: P3 → P2
Is there any chance of providing any form of Domain to IP conversion? It can be fresh DNS query or even grabbing the IP from the Firefox DNS cache. Domain based proxy selection, while mostly adequate, does not cover all the required possibilities. The PAC implementation (direct via Network - Connections - Settings) seems to use a C++ synchronous dnsResolve. As mentioned in comment 7, there is API access to asyncResolve() (and I have already prepared the code) but that may or may not work in WebExtension implementation of Proxy API. It appears that the WebExtension Proxy API is also synchronous. Is there any way to pass the IP to ProxyScriptContext.jsm?
> Is there any chance of providing any form of Domain to IP conversion? ... Is there any way to pass the IP to ProxyScriptContext.jsm? I think you should open a new bug
> I think you should open a new bug Done ... bug 1382684
moving this up in prio since without these functions the pac scripts in webextensions are truly incompatible.
Assignee: nobody → mixedpuppy
Priority: P2 → P1
Attachment #8902783 - Flags: feedback?(kmaglione+bmo)
Attachment #8902783 - Flags: review?(kmaglione+bmo)
Comment on attachment 8902783 [details] Bug 1353510 add PAC script API for compatibility, oops.
Attachment #8902783 - Flags: review?(kmaglione+bmo)
(In reply to erosman from comment #11) > The PAC implementation (direct via Network - Connections - Settings) seems > to use a C++ synchronous dnsResolve. > > As mentioned in comment 7, there is API access to asyncResolve() (and I have > already prepared the code) but that may or may not work in WebExtension > implementation of Proxy API. The call into the PAC script is synchronous no matter what, so an async dns resolve wont work there. IMO PAC scripts are only near future and we need to move to a real async proxy filter, but that is deeper and more involved.
> IMO PAC scripts are only near future and we need to move to a real async proxy filter, but that is deeper and more involved. This.
> IMO PAC scripts are only near future and we need to move to a real async proxy filter, but that is deeper and more involved. True... PACs are really old school, although they still work and on occasions (limited though) work better than the modern Proxy. Most of its features are covered or can be incorporated into the modern 'webRequest' API with some added features (one of which is passing the IP to the API as in Bug 1382684 ). Passing the local IP (not that necessary!!) and target IP to 'webRequest' API enables nearly all required functionally of PAC within 'webRequest'. There would be no need for a DNS request and DNS API for the addon, in that case. To clarify, if browser can pass the IP (as already retrieved by the browser for 'webRequest') and pass it to the 'webRequest', along with the domain; that should be all needed to replace sync PAC with async 'webRequest'. Once that is done, if there is a need to have a PAC feature by the browser, it can be passed to 'webRequest' for async operation. The rest of PAC functions are just pure JS functions that can be done from the developer side (if ever needed). One other valuable PAC feature is the ability to pass authorisation request handling to Firefox which helps in censorship situations. While on PAC issue, please also note the Bug 1378205 and the ambiguity and inconsistency about SOCKS/SOCKS5
(In reply to erosman from comment #21) > To clarify, if browser can pass the IP (as already retrieved by the browser > for 'webRequest') and pass it to the 'webRequest', along with the domain; > that should be all needed to replace sync PAC with async 'webRequest'. I don't see how, since none of the proxy handling code in any channel would work.
Comment on attachment 8902783 [details] Bug 1353510 add PAC script API for compatibility, https://reviewboard.mozilla.org/r/174450/#review183082 Sorry, but synchronous DNS resolution on the main thread is a non-starter.
Attachment #8902783 - Flags: review?(kmaglione+bmo) → review-
hard to be compatible with a synchronous system. dropping priority to rethink.
Priority: P1 → P3
You can always return DNS results if they're already cached and null otherwise.
Switched to using OFFLINE for dnsResolve, cleaned up js in the pac string to pass eslint (if it were not a string).
Comment on attachment 8902783 [details] Bug 1353510 add PAC script API for compatibility, Due to bug 1398646 one of the major components here is no longer possible. As such, and that much of the other code can be done without our help, closing this.
Attachment #8902783 - Flags: review?(kmaglione+bmo)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: