Closed Bug 1425535 Opened 6 years ago Closed 6 years ago

proxy.setConfig for proxy prefs

Categories

(WebExtensions :: Request Handling, enhancement, P3)

Product:
WebExtensions
Component:
Request Handling
Version:
58 Branch
Type:
enhancement

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: mixedpuppy, Assigned: bsilverberg)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

Bug 1425535 - Implement browserSettings.proxyConfig API,
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details 
This is for the proxy settings in about:preferences.  We will allow one extension to control those prefs exposed in UI.  The settings precedence will be used to choose which one, and users will be able to select a different extension via the prefs UI.
Priority: -- → P3
Depends on: 1428486
Blocks: 1428486
No longer depends on: 1428486
See Also: → 1429544
See Also: 1429544 →
No longer blocks: 1428486
Blocks: 1429593
I created bug 1429593 to track the Jazz portion of this bug. This bug will now be used just to track the API implementation. Based on our meeting today, we decided that the API will be called browserSettings.proxyConfig, which will accept an object that will have properties for all of the prefs exposed in the "Connection Settings" panel of about:preferences. Only one extension will be able to control these at one time, using the ExtensionSettingsStore to track precedence.
straw man config object

let proxyConfig = {
  // network.proxy.type, pref is integer but I think we should use string in our api
  type: enum[none, autoDetect, system, manual, autoConfig], 

  // splits into network.proxy.http and .http_port, type==manual
  http: "https://someproxy.com:8888", 

  // network.proxy.share_proxy_settings I think, type==manual
  httpProxyAll: boolean 

  // splits into network.proxy.ssl and .ssl_port, type==manual
  ssl: "https://someproxy.com:8888", 

  // splits into network.proxy.ftp and .ftp_port, type==manual
  ftp: "https://someproxy.com:7777", 

  // splits into network.proxy.socks and .socks_port, type==manual
  socks: "socksproxy:6666", 

  // network.proxy.socks_version, type==manual
  socksVersion: 5, 

  // becomes network.proxy.no_proxies_on, type==manual
  passthrough: ".mozilla.org 192.168.1.0/24", 

  // network.proxy.autoconfig_url, type==autoConfig
  autoConfigUrl: "http://site/autoconfig",

  // network.proxy.socks_remote_dns, type != none
  proxyDNS: boolean, 

  // signon.autologin.proxy, type != none
  autoLogin: boolean
}

There are other settings, but I think this covers everything that is available in preferences.
I have included some basic validations of the input config object, which include:

- Using nsIURI to ensure that any value passed for http, ftp, ssl, socks or autoConfigUrl is a valid url.
- Ensuring that a valid URL is passed into autoConfigUrl if the proxyType is autoConfig.
- Ensuring that proxyType is a valid value.
- Ensuring that socksVersion is either 4 or 5.

I'm not sure what other validations, if any, are necessary.
Comment on attachment 8942337 [details]
Bug 1425535 - Implement browserSettings.proxyConfig API,

https://reviewboard.mozilla.org/r/212616/#review218672

Primary issue is that network.proxy.socks is a host rather than schema://host

::: toolkit/components/extensions/ext-browserSettings.js:168
(Diff revision 3)
> +      "network.proxy.share_proxy_settings": value.httpProxyAll,
> +      "network.proxy.socks_version": value.socksVersion,
> +      "network.proxy.no_proxies_on": value.passthrough,
> +    };
> +
> +    for (let prop of ["http", "ftp", "ssl", "socks"]) {

socks will only use a host for the pref, not a url.  so the setting would look like proxy.com:1234 or 1207.0.0.1:8888 so you'd need to special case with  something like:

uri = Services.io.newURI(`socks://${value[prop]}`);
prefs[`network.proxy.${prop}`] = uri.host;

OR (and I think I prefer the above)

We document that the socks setting done via webextensions must be in the form of socks://ip_or_domain:port

But we still have to make sure that the pref gets set to uri.host.

::: toolkit/components/extensions/ext-browserSettings.js:280
(Diff revision 3)
> +                httpProxyAll: Services.prefs.getBoolPref("network.proxy.share_proxy_settings"),
> +                socksVersion: Services.prefs.getIntPref("network.proxy.socks_version"),
> +                passthrough: Services.prefs.getCharPref("network.proxy.no_proxies_on"),
> +              };
> +
> +              for (let prop of ["http", "ftp", "ssl", "socks"]) {

special case for socks again, and possibly elsewhere below.

::: toolkit/components/extensions/schemas/browser_settings.json:57
(Diff revision 3)
> +              "autoConfig"
> +            ],
> +            "description": "The type of proxy to use."
> +          },
> +          "http": {
> +            "type": "string",

Another thought, we could just be doing the validation here.  Add a origin and host format function to FORMATS in Schema.jsm, and the cooresponding format for the values here.  You wouldn't need the validation in the api code.  They could look something like:

origin(string) {
  let url = new URL(string);
  return {
    schema: url.protocol,
    host: url.hostname,
    port: url.port,
    origin: url.origin
    }
}

// used for sockshost
host(string) {
  // add http protocol just to make URL parse for us.
  let url = new URL(`http://${string}`);
  return {
    hostname: url.hostname,
    port: url.port
    host: url.host
    }
}

::: toolkit/components/extensions/schemas/browser_settings.json:88
(Diff revision 3)
> +            "optional": true,
> +            "description": "The version of the socks proxy.",
> +            "enum": [4, 5]
> +          },
> +          "passthrough": {
> +            "type": "string",

This could be an array of string, but I dont feel strongly about it.  Documentation (on MDN at least) should reflect what can be used here.

::: toolkit/components/extensions/schemas/browser_settings.json:93
(Diff revision 3)
> +            "type": "string",
> +            "optional": true,
> +            "description": "A list of hosts which should not be proxied."
> +          },
> +          "autoConfigUrl": {
> +            "type": "string",

add format: "url" and you dont need the validation in the api code.
Attachment #8942337 - Flags: review?(mixedpuppy) → review-
Comment on attachment 8942337 [details]
Bug 1425535 - Implement browserSettings.proxyConfig API,

https://reviewboard.mozilla.org/r/212616/#review218672

> Another thought, we could just be doing the validation here.  Add a origin and host format function to FORMATS in Schema.jsm, and the cooresponding format for the values here.  You wouldn't need the validation in the api code.  They could look something like:
> 
> origin(string) {
>   let url = new URL(string);
>   return {
>     schema: url.protocol,
>     host: url.hostname,
>     port: url.port,
>     origin: url.origin
>     }
> }
> 
> // used for sockshost
> host(string) {
>   // add http protocol just to make URL parse for us.
>   let url = new URL(`http://${string}`);
>   return {
>     hostname: url.hostname,
>     port: url.port
>     host: url.host
>     }
> }

The reason I added all the validations to the API code is that we don't get any schema validations for this, because we don't get schema validations for the `types.Setting` type. This is bug 1421648, and maybe we should just fix that bug now, so we can have schema validations. It's not a small bug, but probably worth doing now, rather than hacking something together for this bug. What do you think?
(In reply to Bob Silverberg [:bsilverberg] from comment #8)
> Comment on attachment 8942337 [details]
> Bug 1425535 - Implement browserSettings.proxyConfig API,

> The reason I added all the validations to the API code is that we don't get
> any schema validations for this, because we don't get schema validations for
> the `types.Setting` type. This is bug 1421648, and maybe we should just fix
> that bug now, so we can have schema validations. It's not a small bug, but
> probably worth doing now, rather than hacking something together for this
> bug. What do you think?

No.  I spaced out that we didn't have the validation.  

Lets get this in, it's a good improvement for some proxy use cases.  The socks host change does need to happen.
Comment on attachment 8942337 [details]
Bug 1425535 - Implement browserSettings.proxyConfig API,

https://reviewboard.mozilla.org/r/212616/#review219040
Attachment #8942337 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8942337 [details]
Bug 1425535 - Implement browserSettings.proxyConfig API,

https://reviewboard.mozilla.org/r/212616/#review219346

::: toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js:182
(Diff revisions 5 - 6)
>    await testSetting(
>      "openSearchResultsInNewTabs", false,
>      {"browser.search.openintab": false});
>  
>    async function testProxy(config, expectedPrefs) {
> +    // proxyConfig is not supported on Android.

Shouldn't this just be in the ini file?  Also should the api itself throw an error or warning?
Comment on attachment 8942337 [details]
Bug 1425535 - Implement browserSettings.proxyConfig API,

https://reviewboard.mozilla.org/r/212616/#review219352
(In reply to Shane Caraveo (:mixedpuppy) from comment #14)

> >    async function testProxy(config, expectedPrefs) {
> > +    // proxyConfig is not supported on Android.
> 
> Shouldn't this just be in the ini file?  Also should the api itself throw an
> error or warning?

rb didn't post my followup.  realized soon as I hit publish there are other tests that probably need to run.  Still wondering if the api should warn.
(In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #14)
> 
> > >    async function testProxy(config, expectedPrefs) {
> > > +    // proxyConfig is not supported on Android.
> > 
> > Shouldn't this just be in the ini file?  Also should the api itself throw an
> > error or warning?
> 
> rb didn't post my followup.  realized soon as I hit publish there are other
> tests that probably need to run.  Still wondering if the api should warn.

Maybe we should warn/throw an error. I wanted to ask you if it is meant to be supported on Android or not, but didn't see you on IRC and wanted to try to get a clean try run today. 

I guess if we're assuming it's unsupported on Android for now, we should throw an error if someone tries to call it. I'll make that change.
(In reply to Bob Silverberg [:bsilverberg] from comment #17)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #14)
> > 
> > > >    async function testProxy(config, expectedPrefs) {
> > > > +    // proxyConfig is not supported on Android.
> > > 
> > > Shouldn't this just be in the ini file?  Also should the api itself throw an
> > > error or warning?
> > 
> > rb didn't post my followup.  realized soon as I hit publish there are other
> > tests that probably need to run.  Still wondering if the api should warn.
> 
> Maybe we should warn/throw an error. I wanted to ask you if it is meant to
> be supported on Android or not, but didn't see you on IRC and wanted to try
> to get a clean try run today. 
> 
> I guess if we're assuming it's unsupported on Android for now, we should
> throw an error if someone tries to call it. I'll make that change.

This is a bit more complicated that I thought, because of the implementation of browserSettings and the shared code. We are currently overriding `set` so it's simple to make that throw on Android, but we're not overriding `get` or `clear`, so that's more complicated. It looks like we have a couple of options:

1. Explicitly override `get` and `clear` in the definition of proxyConfig as we're currently doing with `set`, adding code that throws if we're on Android.
2. Enhance the `getSettingsAPI` function to accept a list of unsupported platforms, and add code into the methods in `getSettingsAPI` to throw if the call is made on an unsupported platform.

#2 will futureproof us for cases where we have additional settings that are not supported on particular platforms, but, because we're already overriding `set` for proxyConfig, we'd have to add a check there too.

We also should decide if this should be a warning or an error.

What do you think, Shane?
Flags: needinfo?(mixedpuppy)
(In reply to Bob Silverberg [:bsilverberg] from comment #19)
> Comment on attachment 8942337 [details]
> Bug 1425535 - Implement browserSettings.proxyConfig API,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/212616/diff/6-7/

Looks like you went with #2.  interdiff LGTM.
Flags: needinfo?(mixedpuppy)
(In reply to Shane Caraveo (:mixedpuppy) from comment #20)
> (In reply to Bob Silverberg [:bsilverberg] from comment #19)
> > Comment on attachment 8942337 [details]
> > Bug 1425535 - Implement browserSettings.proxyConfig API,
> > 
> > Review request updated; see interdiff:
> > https://reviewboard.mozilla.org/r/212616/diff/6-7/
> 
> Looks like you went with #2.  interdiff LGTM.

I know we want to land this for 59 if possible, so I went with #2 proactively, with a willingness to change if you said differently. I've asked Luca to run the Android test for me to verify it's okay, and I've also requested a new try run.

Thanks for the reviews.
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/891afc6d66f1
Implement browserSettings.proxyConfig API, r=mixedpuppy
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/891afc6d66f1
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(bob.silverberg)
Flags: needinfo?(bob.silverberg) → qe-verify-
I did notice the dev-doc-needed :)

For the time being, it seems `browserSettings.proxyConfig.set` is using keywords for `proxyType` (in comparison to numbers and different keywords in `network.proxy.type`)

Are these the options? 
proxyType: "none" "autoDetect" "system" "autoConfig" "manual"
PS. Looking at (test_ext_browserSettings.js), any chance of adding a test for a local filesystem autoConfigUrl (e.g. file:///....)
It should not be a problem but worth testing.
(In reply to erosman from comment #27)
> I did notice the dev-doc-needed :)
> 
> For the time being, it seems `browserSettings.proxyConfig.set` is using
> keywords for `proxyType` (in comparison to numbers and different keywords in
> `network.proxy.type`)
> 
> Are these the options? 
> proxyType: "none" "autoDetect" "system" "autoConfig" "manual"

That is correct. You can see the options at https://searchfox.org/mozilla-central/rev/e3cba77cee3ff1be38313abe9c804d13c51bd95b/toolkit/components/extensions/schemas/browser_settings.json#44-55.
(In reply to erosman from comment #28)
> PS. Looking at (test_ext_browserSettings.js), any chance of adding a test
> for a local filesystem autoConfigUrl (e.g. file:///....)
> It should not be a problem but worth testing.

Patches are welcome.
re: reload PAC (ref: bug 1412790#c23)

I believe Firefox caches the PAC (I don't know how i.e. at start up or indefinitely like the changes in FF55+ bug 1356826).

What happens in case of Dynamic PACs? 
The URL stays the same so the automatic reload wont be called and a manual Reload would be necessary.
Flags: needinfo?(mixedpuppy)
The only options (related to pac) in prefs that we are handling is the type and autoconfig_url.  Per nsProtocolProxyService.cpp#789, if either of those change, the pac is reloaded.  There is no concept of dynamic pacs here, that seems like something in your extension, and in this case unrelated to changing prefs.  If it's a feature request then make a new bug.  If you're convinced that changing prefs somehow creates a dynamic pac, then better information is needed.
Flags: needinfo?(mixedpuppy)
Shane: After some testing, I wanted to confirm that adding search query string to the PAC URL (eg: .../test.pac?1) does cause it to be reloaded without any side-effect that I can see.
Ok, so it's really because the pref didn't in fact change, so the pac is not reloaded.
For the time being, since the PAC reloads with new query string, I think that should be fine.
Added a page on this: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserSettings/proxyConfig

Please let me know if this covers it.
Flags: needinfo?(bob.silverberg)
- Just out of curiosity (not that important, just for clarity), is the function sync or async? It doesn't say anything on above MDN page.

- BTW, BrowserSetting page also requires an update
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/types/BrowserSetting

- Does it require any "permissions"?
> is the function sync or async? It doesn't say anything on above MDN page.

It's implemented as a "BrowserSetting", which is an object with "set()" and "get()" functions, that are both async. This is documented in the page describing the BrowserSetting type: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/types/BrowserSetting. 

> - BTW, BrowserSetting page also requires an update
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/types/BrowserSetting

I'm not sure what update is needed here.

> - Does it require any "permissions"?

It's part of the "browserSettings" API, that requires the "browserSettings" permission. This is documented in the page for the "browserSettings" API: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserSettings
(In reply to Will Bamberg [:wbamberg] from comment #36)
> Added a page on this:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/
> browserSettings/proxyConfig
> 
> Please let me know if this covers it.

Looks good, thanks Will.
Flags: needinfo?(bob.silverberg)
Hello,

Can somebody take a look at https://github.com/rNeomy/proxy-switcher/issues/46

A user needs to set the HTTP proxy to an IP address without the scheme. Basically, when the scheme is added (I've tried socks://, http:// and https://), the server is not responding. As far as I can see it is not doable to set the HTTP proxy without the scheme from a web extension, right?
Flags: needinfo?(ddurst)
Depends on: 1455705
Product: Toolkit → WebExtensions
Flags: needinfo?(thedurst)
See Also: → 1725981
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: