Closed
Bug 1353510
Opened 8 years ago
Closed 7 years ago
Add support for the global PAC functions
Categories
(WebExtensions :: Request Handling, enhancement, P3)
WebExtensions
Request Handling
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: mattw, Assigned: mixedpuppy)
References
(Blocks 1 open bug)
Details
(Whiteboard: [proxy] triaged)
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
Details |
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
Updated•7 years ago
|
Keywords: dev-doc-needed
Comment 1•7 years ago
|
||
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.
Reporter | ||
Comment 6•7 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.
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.
Comment 9•7 years ago
|
||
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
Comment 10•7 years ago
|
||
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
Updated•7 years ago
|
Priority: P3 → P2
Comment 11•7 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?
Comment 12•7 years ago
|
||
> 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•7 years ago
|
||
> I think you should open a new bug
Done ... bug 1382684
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•7 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•7 years ago
|
Attachment #8902783 -
Flags: feedback?(kmaglione+bmo)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8902783 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 17•7 years ago
|
||
Comment on attachment 8902783 [details]
Bug 1353510 add PAC script API for compatibility,
oops.
Attachment #8902783 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 19•7 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.
Comment 20•7 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.
This.
Comment 21•7 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•7 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•7 years 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•7 years ago
|
||
hard to be compatible with a synchronous system. dropping priority to rethink.
Priority: P1 → P3
Comment 25•7 years ago
|
||
You can always return DNS results if they're already cached and null otherwise.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 27•7 years 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•7 years 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•7 years ago
|
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Comment 29•5 years ago
|
||
PAC are already documented as deprecated.
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/proxy/register#PAC_file_specification
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•