Add support for the global PAC functions

RESOLVED WONTFIX

Status

P3
normal
RESOLVED WONTFIX
2 years ago
8 months ago

People

(Reporter: mattw, Assigned: mixedpuppy)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [proxy] triaged)

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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
Keywords: dev-doc-needed
These implementations can be used in the WebExtensions Proxy API:
https://dxr.mozilla.org/mozilla/source/netwerk/base/src/nsProxyAutoConfig.js

Comment 2

2 years ago
Is nsProxyAutoConfig.js available in FF55+?
Can it be imported into nsProxyAutoConfig.js?
What about ProxyAutoConfig.cpp?

Comment 3

2 years ago
I have started to write the PAC functions. Once finished, I will pass it Matthew for testing and implementation.

Comment 4

2 years ago
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)

Comment 5

2 years ago
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.
(Reporter)

Comment 6

2 years ago
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.

Comment 7

2 years ago
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()

Comment 8

2 years ago
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

Comment 11

2 years ago
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

Comment 13

2 years ago
> I think you should open a new bug

Done ... bug 1382684
Comment hidden (mozreview-request)
(Assignee)

Comment 15

2 years ago
moving this up in prio since without these functions the pac scripts in webextensions are truly incompatible.
Assignee: nobody → mixedpuppy
Priority: P2 → P1
(Assignee)

Updated

2 years ago
Attachment #8902783 - Flags: feedback?(kmaglione+bmo)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8902783 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 17

2 years ago
Comment on attachment 8902783 [details]
Bug 1353510 add PAC script API for compatibility,

oops.
Attachment #8902783 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1382684
(Assignee)

Comment 19

2 years ago
(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.

Comment 21

2 years ago
>  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
(Assignee)

Comment 22

2 years ago
(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 23

a year ago
mozreview-review
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-
(Assignee)

Comment 24

a year ago
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.
Comment hidden (mozreview-request)
(Assignee)

Comment 27

a year ago
Switched to using OFFLINE for dnsResolve, cleaned up js in the pac string to pass eslint (if it were not a string).
(Assignee)

Comment 28

a year ago
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)
(Assignee)

Updated

a year ago
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → WONTFIX

Updated

8 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.