proxy.setConfig for proxy prefs

RESOLVED FIXED in Firefox 59

Status

enhancement
P3
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: mixedpuppy, Assigned: bsilverberg, NeedInfo)

Tracking

({dev-doc-complete})

58 Branch
mozilla59
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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.

Updated

a year ago
Priority: -- → P3

Updated

a year ago
Depends on: 1428486
(Assignee)

Updated

a year ago
Blocks: 1428486
No longer depends on: 1428486
(Reporter)

Updated

a year ago
See Also: → 1429544
(Reporter)

Updated

a year ago
See Also: 1429544
(Reporter)

Updated

a year ago
No longer blocks: 1428486
(Assignee)

Updated

a year ago
Blocks: 1429593
(Assignee)

Comment 1

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

Comment 2

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

Comment 6

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

Comment 7

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

Comment 8

a year ago
mozreview-review-reply
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?
(Reporter)

Comment 9

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

Comment 11

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

Comment 14

a year ago
mozreview-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?
(Reporter)

Comment 15

a year ago
mozreview-review
Comment on attachment 8942337 [details]
Bug 1425535 - Implement browserSettings.proxyConfig API,

https://reviewboard.mozilla.org/r/212616/#review219352
(Reporter)

Comment 16

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

Comment 17

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

Comment 18

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

Comment 20

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

Comment 21

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

Comment 23

a year ago
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/891afc6d66f1
Implement browserSettings.proxyConfig API, r=mixedpuppy
(Assignee)

Updated

a year ago
Keywords: dev-doc-needed

Comment 24

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/891afc6d66f1
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59

Comment 25

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

Updated

a year ago
Flags: needinfo?(bob.silverberg) → qe-verify-
(Reporter)

Updated

a year ago
Duplicate of this bug: 1412790

Comment 27

a year ago
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"

Comment 28

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

Comment 29

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

Comment 30

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

Comment 31

a year ago
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.

Updated

a year ago
Flags: needinfo?(mixedpuppy)
(Reporter)

Comment 32

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

Comment 33

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

Comment 34

a year ago
Ok, so it's really because the pref didn't in fact change, so the pac is not reloaded.

Comment 35

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

Comment 37

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

Comment 39

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

Comment 40

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

Updated

a year ago
Depends on: 1455705

Updated

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