Closed Bug 1283160 (webext-port-hola) Opened 8 years ago Closed 7 years ago

Enable Unlimited Free VPN - Hola Chrome extension to work with Firefox

Categories

(WebExtensions :: Compatibility, defect, P5)

Product:
WebExtensions
Component:
Compatibility
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: bsilverberg, Unassigned)

References

(Depends on 1 open bug)

Details

(Whiteboard: [popular chrome extensions]triaged) 

Chrome extension at https://chrome.google.com/webstore/detail/unlimited-free-vpn-hola/gkojfkhlekighikafcpjkiklfbnlmeio
Test uploaded to AMO at https://addons-dev.allizom.org/en-US/firefox/addon/unlimited-free-vpn-hola/

Missing APIs:
- chrome.runtime.onInstalled (bug 1252871)

Note also that the copy currently on AMO for testing appears to be corrupt when attempting to install into Firefox Nightly.
This very likely also depends on chrome.proxy
Grepping through the code shows that you are correct. I actually only see one use of `chrome.proxy` which is a check to see if `chrome.proxy` exists, but there is a lot of other code that deals with proxy settings and for now we should assume that it makes use of `chrome.proxy`. Thanks Johann!
Having grepped the full source code, here is a list of missing APIs:

- chrome.app.getDetails
- chrome.devtools.network
- chrome.extension.onConnectExternal (aliased to runtime)
- chrome.extension.onMessageExternal (aliased to runtime)
- chrome.proxy
- chrome.runtime.onInstalled
- chrome.runtime.requestUpdateCheck
- chrome.webRequest.onAuthRequired
Alias: webext-port-hola
Depends on: 1258360
Some of the code is obfuscated, and it looks like that code is what is using chrome.proxy.  It renames the chrome namespace to "B", so using that I found the following proxy components used:

 - chrome.proxy.settings.get
 - chrome.proxy.settings.set
 - chrome.proxy.settings.clear
 - chrome.proxy.settings.onChange
Component: WebExtensions: Untriaged → WebExtensions: Compatibility
Priority: -- → P5
Depends on: 1295807
No longer depends on: webextensions-proxy-api
No longer depends on: 1295807
Proxy support landed in 55. However it is different from Chrome so a direct copy of this add-on won't work. But it should now be possible.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Hi, I am a maintainer for HolaVpn, we have some problems with our extension in the new firefox versions and would like for some help.

We are currently running in hybrid mode, we rely on the legacy addons api for setting the pac script, and everything else works with webextension api.

Our extension works per tab, and also filters out some requests (like video/audio), so we are sending config to the pac script in webRequest.onBeforeRequest for every request.

Since firefox 54, our extension does not work for all CORS requests for the following reason:
- The pac script is accessed before the webRequest.onBeforeRequest hook, so the config we send to the pac script is useless
- We previously solved it by doing a redirect of the request on itself in webRequest api, but with firefox 54 release, thous 
  redirects are blocked by CORS (event though the url does not change and CORS headers are present)

We thought that migrating to the new proxy api in firefox 55 will solve our problem, but it did not, it only added:
- the pac script is still accessed before the onBeforeRequest hook so our CORS problem remains
- the proxy api does not allow setting pac script dynamically (via data uri)
- global pac functions are not implemented yet (https://bugzilla.mozilla.org/show_bug.cgi?id=1353510)
- path is truncated for http urls in pac script (https://bugzilla.mozilla.org/show_bug.cgi?id=1337001)

So the question is, what we can currently do to fix our CORS? Thous issues seem like actual bugs in firefox.

(In reply to Andy McKay [:andym] from comment #5)
> Proxy support landed in 55. However it is different from Chrome so a direct
> copy of this add-on won't work. But it should now be possible.
> - the pac script is still accessed before the onBeforeRequest hook so our
> CORS problem remains
Thanks for pointing this out - I will file a bug for this so we can track fixing it. In the meantime, I think you could make a round-trip request to the background script to obtain the config as a workaround.

> - the proxy api does not allow setting pac script dynamically (via data uri)
Could you please explain what you need this functionality for?
> > - the proxy api does not allow setting pac script dynamically (via data uri)
> Could you please explain what you need this functionality for?

We use it mainly for initial configuration. It is not a show stopper, but it will help us to remain cross platform without the usage of the non standard communication methods. That's how we use it in chrome and in the legacy Firefox api.


> I think you could make a round-trip request to the background script to obtain the config as a workaround
You mean a request from the pac script to background via the messaging api?
If so, I don't believe it can work unless firefox pac script allows async requests.
> > I think you could make a round-trip request to the background script to obtain the config as a workaround
> You mean a request from the pac script to background via the messaging api?
> If so, I don't believe it can work unless firefox pac script allows async requests.

Also, I don't know the context of the request. So I will not be able to catch any CORS requests.
> Also, I don't know the context of the request. So I will not be able to
> catch any CORS requests.

I see, thanks for the details. I think we'll need to get onBeforeRequest running before the proxy scripts execute then. I'll file a bug for this.
Thanks!

(In reply to Matthew Wein [:mattw] from comment #10)
> > Also, I don't know the context of the request. So I will not be able to
> > catch any CORS requests.
> 
> I see, thanks for the details. I think we'll need to get onBeforeRequest
> running before the proxy scripts execute then. I'll file a bug for this.
> The pac script is accessed before the webRequest.onBeforeRequest hook, so the config we send to the pac script is useless

I don't know what this configuration is, but why aren't you sending it using runtime.sendMessage()?

> global pac functions are not implemented yet (https://bugzilla.mozilla.org/show_bug.cgi?id=1353510)

You can easily implement them yourself by copying the implementations into your FindProxyForPAC() module. It's important to realize this PAC file is not a typical server-hosted PAC file and was never intended to be anything but a callback for the question "what proxy should this URL load through, if any?"

> but it will help us to remain cross platform without the usage of the non standard communication methods. That's how we use it in chrome and in the legacy Firefox api.

What is a "non standard communication method". Please explain. Separately, please provide an example of how you do this in the legacy Firefox API.
(In reply to Matthew Wein [:mattw] from comment #10)
> > Also, I don't know the context of the request. So I will not be able to
> > catch any CORS requests.
> 
> I see, thanks for the details. I think we'll need to get onBeforeRequest
> running before the proxy scripts execute then. I'll file a bug for this.

I think it's likely the other way around, make proxy scripts execute after onBeforeRequest.  There isn't a way to run onBeforeRequest earlier than what it does now without breaking compat.  

If that is not possible, then we'd have to add a new webrequest event that fires on the http-on-opening-request notification.
> I'll file a bug for this.
bug 1381070
> > The pac script is accessed before the webRequest.onBeforeRequest hook, so the config we send to the pac script is useless

> I don't know what this configuration is, but why aren't you sending it using runtime.sendMessage()?

We want to make decisions on every request before it is fired and notify the pac script accordingly. So it does not matter which communication method we use, we don't have a hook to tie it to before proxy script is executed.

The other issue with runtime.sendMessage() is that it is async, so we cannot execute it on before request and be sure that the pac script was updated before request continues.

> You can easily implement them yourself by copying the implementations into your FindProxyForPAC() module. It's important to 
> realize this PAC file is not a typical server-hosted PAC file and was never intended to be anything but a callback for the 
> question "what proxy should this URL load through, if any?"

Yes some of them we can, and it is not a real issue. There is no critical functionality that depends on it. But we cannot implement a dns resolution on our own as far as I know.

> What is a "non standard communication method"

runtime.sendMessage is non standard for pac scripts, it is better and cleaner, but 
sadly does not suit our use case because we need a sync way. Also it will mean that we cannot use the same pac script for all platforms (which is not a big deal really)

> please provide an example of how you do this in the legacy Firefox API

In legacy api we do the proxy decisions in on_opening_request hook of sdk/system/events

We generate the pac script and set it to network.proxy.autoconfig_url as data uri

We communicate with the pac script by sending a sync ajax request from extension to a special domain local.hola which is handled by our pac script.
> We want to make decisions on every request before it is fired and notify the pac script accordingly.

This decision should be done in the PAC module itself. That is the point of the PAC module. You should have access to all data needed to make such decisions. If you do not, please explain what that data is missing (but note bug 1337001 and bug 1368559 are not completed).

> So it does not matter which communication method we use, we don't have a hook to tie it to before proxy script is executed.

You do. You have runtime.sendMessage().

> The other issue with runtime.sendMessage() is that it is async so we cannot execute it on before request
> and be sure that the pac script was updated before request continues.

This is a circular argument and yet another reason why you should be making proxying decisions in FindProxyForURL() and nowhere else. By the way, FindProxyForURL() is executed synchronously. You cannot make async calls from it. This is limitation of gecko, not the API itself. The bottom line is you should be using runtime.sendMessage() to send the FindProxyForURL() module whatever data and state it needs to make proxying decisions for the lifetime of execution.

> But we cannot implement a dns resolution on our own as far as I know.

That is correct. Performing DNS resolution within FindProxyForURL() would slow things down: remember, FindProxyForURL() is synchronous. You can perform the DNS resolution elsewhere and use runtime.sendMessage() to send the resolution to the FindProxyForURL() module.

> runtime.sendMessage is non standard for pac scripts

As I wrote at https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/proxy: "This implementation deviates from standard PAC design in several ways because the de-facto specification for PAC files hasn't changed since its initial implementation circa 1995. There is no standards body maintaining the specification." Having said that, I do regret naming this function FindProxyForURL() because people now have unintended expectations. A better name would have been getProxyForURI() to indicate (1) it does not follow any previous standard of any kind; (2) it has no similarity to PAC files except to answer the question "should I use a proxy for this URL?"; (3) it is not server-hosted like most PAC files.

> but sadly does not suit our use case because we need a sync way.

You need to be more creative in the way you are writing your addon. My guess is that you are trying to port a Chrome addon directly to Firefox without understanding the proxy API differences between the two. You still have not explained why you must send configuration changes to the PAC module for each and every web request instead of sending that configuration info once at startup (or at other key events). The only reason I can think of is that you are in constant communication with some remote server, perhaps retrieving updated proxy lists from it. If that's the case, you shouldn't be doing that before every new request.

> Also it will mean that we cannot use the same pac script for all platforms (which is not a big deal really)

By platforms, I assume you mean browsers. It is not Mozilla's goal to copy Chrome extension APIs one-for-one, especially when the Chrome APIs are poorly designed. It is Mozilla's goal to innovate where appropriate. This is one such area.

> In legacy api we do the proxy decisions in on_opening_request hook of sdk/system/events

That's the wrong place. You should be doing it in nsIProtocolProxyFilter.applyFilter().

> We generate the pac script and set it to network.proxy.autoconfig_url as data uri
> We communicate with the pac script by sending a sync ajax request from extension to a special domain local.hola which is handled by our pac script.

That is ... interesting. I wonder how that affects performance; it certainly can't be good. None of that is needed if you'd used nsIProtocolProxyFilter.applyFilter(). And now you want to inject the same poor design into WebExentensions APIs.
Hi thank you for taking the time to understand our problem.

> This decision should be done in the PAC module itself. That is the point of the PAC module.

I can see it from the theoretical point of view. But in practice, for our extension it will be more complicated if this is the case.
- we will have to maintain duplicate state
- we will have to be very chatty with the pac script to update this state in two way communication
- we will have to simplify our state haling very match because the pac module should work very fast and not block things
- the pac script does not have access to any apis (like local storage, settings, whatever we might need, all of this will have 
  to be more complex to handle)
- we would have to avoid any async staff in the pac file

All of this can be solved ofcource, but I don't see a real reason why we have to. We are doing a simple "set_proxy_for_url" message and forget about it.

As you surly know any state duplication is always a big complication.

> > We want to make decisions on every request before it is fired and notify the pac script accordingly.
> This decision should be done in the PAC module itself.

Currently the data from  bug 1337001 and bug 1368559 is not available in the pac script and it is unclear when it will be.
When it does though it will be enough for most of our use-cases. 

> If you do not, please explain what that data is missing

In addition to tabId and windowId, we are currently using requestId. This is needed to track the request trough all of it life-cycle.

For example, we sometimes do different proxy decisions if we redirect the request. There are other use cases as well.

> You do. You have runtime.sendMessage().

It is not a hook, it's a communication method. Right now it does not matter to me which communication method to use, as long as I can use it before the request is run (per request).

> You can perform the DNS resolution elsewhere and use runtime.sendMessage()

How? I could have performed it, if I had onBeforeRequest happen before the pac script was executed, but I don't have a place for it now. When I load a web page, it fires many requests, I want to make decision on each of them, and resolve dns for each as well.

We need it to be able to maintain a black list of sites. Example:

We are not allowing to unblock google trough our extension. But filtering simply by domain is not an option, we had many cases when users configured dns resolution of other domains to google ips.

> The only reason I can think of is that you are in constant communication with some remote server

No, of-course we are not. We are updating proxies on user input. 
We do it today because we cannot tie a request to tab via the pac file (remember we do not have tabId yet, and we cannot wait for it to land).

> My guess is that you are trying port a Chrome addon directly to Firefox

That is incorrect, we are trying to migrate our Firefox addon to the new api, we have a firefox addon for more than 3 years now.

> It is not Mozilla's goal to copy Chrome extension APIs one-for-one, especially when the Chrome APIs are poorly designed
My problem is not with the proxy api right now, more on how it fits the webRequest api.
Let me ask you this. If I decide to cancel the request before it is made, does it seem logical to decide on proxy for it?
Our proxy is a smart proxy, it learns about how to serve a website with minimal proxy traffic.

when we make a request direct/proxy:
1. we might add algorithmic routing headers for proxy server on_before_headers.
2. we monitor the response in on_response_received, cache some algorithmic information for future requests and might re-fire the request again proxy/direct.

moving just the proxy decision into pac will make this process (code) complicated ten-fold:
- it requires keeping all the information related to proxy decisions both in addon  for webRequest api handlers and in pac script for making these first "blind" decisions.
- sync any change with the "other side" using sendMessage.

Another question I have is about the recent change in firefox 54 related to CORS.
Currently if I make a redirect of a cross domain request in onBeforeRequest, my request is blocked due to CORS, even if the redirect I do is to the same url. 

Eric, in bug 1337001 you've written:

> Since addons are trusted, there is no reason to hide path and query components or any other part of the URL from the addon

Is there a reason for CORS requests to be different and not trusted?

> That's the wrong place. You should be doing it in nsIProtocolProxyFilter.applyFilter()

You are correct, my mistake, that's indeed how we did it. We migrated to the other method when we migrated to hybrid extension.
> > We generate the pac script and set it to network.proxy.autoconfig_url as data uri
> > We communicate with the pac script by sending a sync ajax request from extension to a special domain local.hola which is 
> > andled by our pac script.
> That is ... interesting. I wonder how that affects performance; it certainly can't be good. None of that is needed if you'd used 
> nsIProtocolProxyFilter.applyFilter(). And now you want to inject the same poor design into WebExentensions APIs.

- We use ajax for this type of communication in chrome due to their api limitations. The requests are generated in a way that the pac sees them but they fail immediately afterwards in chrome's dns resolution engine with the no need for network traffic (total request time <1ms)
- In firefox, we've always used applyFilter api to make these decisions. Sorry for the misdirection.
A dream solution for us will be throwing away the pac script away all together.
A proxy field in the blockingResponse object returned by onBeforeRequest would do the trick
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.